Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 08:26:44PM GMT, Ilya Maximets wrote:
> On 6/24/24 20:00, Adrián Moreno wrote:
> > On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
> > [...]
> >>
> >> Why are we adding IPFIX?  This should work without IPFIX.
> >>
> >> Having both together can be a separate test.
> >>
> >>> +
> >>> +AT_DATA([flows.txt], [dnl
> >>> +in_port=ovs-p0,ip 
> >>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> >>> +in_port=ovs-p1,ip 
> >>> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
> >>
> >> It should be possible to wrap these lines a little with dnl.
> >
> > I'm curious about wrapping requirements for flows in tests.
> > None of them seem to be wrapped, splitting matches and actions won't
> > make lines much shorter and having more exotic wrapped & indented
> > action arguments seem strage to read (for eyes made to read flows in
> > a single line). What wrapping do you have in mind?
>
> No hard requirements, but I find lines that long hard to read
> since they are going out of the window width for me and getting
> wrapped in random places.
>
>
> AT_DATA([flows.txt], [dnl
> in_port=ovs-p0,ip dnl
> actions=dnl
>  sample( dnl
>   probability=65535, dnl
>   collector_set_id=1,dnl
>   obs_domain_id=1431655765,  dnl
>   obs_point_id=1717986918),  dnl
>  output(port=ovs-p1,max_len=100)
> ...
>
> I agree that above is not very readable...
>
> Or
>
> AT_DATA([flows.txt], [dnl
> m4_join([], [
>   [in_port=ovs-p0,ip], [ ],
>   [actions=],
>
> [sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918)],
>[output(port=ovs-p1,max_len=100)]
> ]
> m4_join([], [
>   [in_port=ovs-p1,ip], [ ],
>   [actions=],
>
> [sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377)],
>[output(port=ovs-p0,max_len=100)]
> ]
> )
>
> Or maybe just shorten the lines with a macro:
>
> m4_define([SAMPLE_ACTION],
>  
> [sample(probability=65535,collector_set_id=$1,obs_domain_id=$2,obs_point_id=$3)]
> )
>
> AT_DATA([flows.txt], [dnl
> in_port=ovs-p0,ip actions=SAMPLE_ACTION(1, 1431655765, 1717986918), 
> output(port=ovs-p1,max_len=100)
> in_port=ovs-p1,ip actions=SAMPLE_ACTION(2, 2290649224, 2576980377), 
> output(port=ovs-p0,max_len=100)
> ])
>

I prefer this one. I'll give it a try.

Thanks.
Adrián

> (Haven't tested any of these)
>
> Best regards, Ilya Maximets.
>

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

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 07:57:28PM GMT, Ilya Maximets wrote:
> On 6/24/24 19:38, Adrián Moreno wrote:
> > On Mon, Jun 24, 2024 at 04:15:50PM GMT, Ilya Maximets wrote:
> >> On 6/24/24 15:14, Adrián Moreno wrote:
> >>> On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
>  On 6/5/24 22:23, Adrian Moreno wrote:
> > Test simultaneous IPFIX and local sampling including slow-path.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
> >  tests/system-common-macros.at |   4 ++
> >  tests/system-traffic.at   | 105 ++
> >  2 files changed, 109 insertions(+)
> >
> 
>  Not a full review, but see a few comments below.
> 
> > diff --git a/tests/system-common-macros.at 
> > b/tests/system-common-macros.at
> > index 2a68cd664..22b8885e4 100644
> > --- a/tests/system-common-macros.at
> > +++ b/tests/system-common-macros.at
> > @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
> >  # OVS_CHECK_DROP_ACTION()
> >  m4_define([OVS_CHECK_DROP_ACTION],
> >  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> > ovs-vswitchd.log])])
> > +
> > +# OVS_CHECK_EMIT_SAMPLE()
> > +m4_define([OVS_CHECK_EMIT_SAMPLE],
> > +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> > ovs-vswitchd.log])])
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index bd7647cbe..babc56b56 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: 
> > * * *5002 *2000 *b85e *00
> >
> >  OVS_TRAFFIC_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([emit_sample])
> 
>  Other tests have a test group name at the beginning.
>  You're also adding the test into NSH section.  Add a new section, e.g.,
>  'sampling' for this with AT_BANNER.
> 
> >>>
> >>> Thanks. Will do.
> >>>
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +OVS_CHECK_EMIT_SAMPLE()
> > +
> > +ADD_NAMESPACES(at_ns0, at_ns1)
> > +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], 
> > [0], [ignore])
> > +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], 
> > [0], [ignore])
> 
>  This looks strange, don't disable IPv6.  Appropriate datapath actions
>  should not forward IPv6 traffic, if that is a desired behavior.
> 
> >>>
> >>> I found tests cleaner if automatic ipv6 traffic is not sent by ports.
> >>> But sure, I can deal with that with OpenFlow.
> >>
> >> Would be good to have two separate tests.  One for IPv4 and one for IPv6.
> >>
> >>>
>  Though, I guess, it would be nice to test capturing both IPv4 and IPv6.
> 
> > +
> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> > +
> > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> > +
> > +
> > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> > ofproto_dpif_upcall:dbg])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +-- --id=@ipfix create IPFIX 
> > targets=\"127.0.0.1:4739\" \
> > +-- create Flow_Sample_Collector_Set id=1 
> > bridge=@br0 ipfix=@ipfix, local_sample_group=10 \
> > +-- create Flow_Sample_Collector_Set id=2 
> > bridge=@br0 ipfix=@ipfix, local_sample_group=12],
> > + [0], [ignore])
> 
>  Why are we adding IPFIX?  This should work without IPFIX.
> 
> >>>
> >>> It is intentional. I want to test both features work simulteaneously.
> >>
> >> Tests should be simple.  At least, we should have at least one very simple
> >> test, so if it fails we know that there is an issue in the basic logic.
> >>
> >> It's valuable to test a combination of the two, but it should be a separate
> >> test.  So, 3 tests so far: IPv4, IPv6, psample+ipfix.  Maybe the 4th 
> >> separate
> >> test for the slow path.
> >>
> >
> > Ack.
> >
> >>>
>  Having both together can be a separate test.
> 
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=ovs-p0,ip 
> > actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> > +in_port=ovs-p1,ip 
> > actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
> 
>  It should be possible to wrap these lines a little with dnl.
> 
>  Also, please make IDs non-symmetric, so we can't check that the
>  byte order is correct.
> 
> >>>
> >>> Originally, I thought just concatenating the values in native endianness
> >>> as a kind of replacement for

Re: [ovs-dev] [PATCH v2 net-next 0/7] selftests: net: Switch pmtu.sh to use the internal ovs script.

[Jakub Kicinski]

On Mon, 24 Jun 2024 12:53:45 -0400 Aaron Conole wrote:
> Additionally, the "Cannot find device ..." text comes from an iproute2
> utility output.  The only place we actually interact with that is via
> the call at pmtu.sh:973:
> 
>   run_cmd ip link set ovs_br0 up
> 
> Maybe it is possible that the link isn't up (could some port memory
> allocation or message be delaying it?) yet in the virtual environment.

Depends on how the creation is implemented, normally device creation
over netlink is synchronous. Just to be sure have you tried to repro
with vng:

https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style

? It could be the base OS difference, too, but that's harder to confirm.

> To confirm, is it possible to run in the constrained environment, but
> put a 5s sleep or something?  I will add the following either as a
> separate patch (ie 7/8), or I can fold it into 6/7 (and drop Stefano's
> ACK waiting for another review):
> 
> 
> wait_for_if() {
>if ip link show "$2" >/dev/null 2>&1; then return 0; fi
> 
>for d in `seq 1 30`; do
>   sleep 1
>   if ip link show "$2" >/dev/null 2>&1; then return 0; fi
>done
>return 1
> }
> 
> 
>   setup_ovs_br_internal || setup_ovs_br_vswitchd || return $ksft_skip
> + wait_for_if "ovs_br0"
>   run_cmd ip link set ovs_br0 up
> 
> 
> Does it make sense or does it seem like I am way off base?

sleep 1 is a bit high (sleep does accept fractional numbers!)
but otherwise worth trying, if you can't repro locally.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next v3 04/10] net: psample: allow using rate as probability

[Ilya Maximets]

On 6/19/24 23:00, Adrian Moreno wrote:
> Although not explicitly documented in the psample module itself, the
> definition of PSAMPLE_ATTR_SAMPLE_RATE seems inherited from act_sample.
> 
> Quoting tc-sample(8):
> "RATE of 100 will lead to an average of one sampled packet out of every
> 100 observed."
> 
> With this semantics, the rates that we can express with an unsigned
> 32-bits number are very unevenly distributed and concentrated towards
> "sampling few packets".
> For example, we can express a probability of 2.32E-8% but we
> cannot express anything between 100% and 50%.
> 
> For sampling applications that are capable of sampling a decent
> amount of packets, this sampling rate semantics is not very useful.
> 
> Add a new flag to the uAPI that indicates that the sampling rate is
> expressed in scaled probability, this is:
> - 0 is 0% probability, no packets get sampled.
> - U32_MAX is 100% probability, all packets get sampled.
> 
> Signed-off-by: Adrian Moreno 
> ---
>  include/net/psample.h |  3 ++-
>  include/uapi/linux/psample.h  | 10 +-
>  include/uapi/linux/tc_act/tc_sample.h |  1 +
>  net/psample/psample.c |  3 +++
>  4 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 2ac71260a546..c52e9ebd88dd 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -24,7 +24,8 @@ struct psample_metadata {
>   u8 out_tc_valid:1,
>  out_tc_occ_valid:1,
>  latency_valid:1,
> -unused:5;
> +rate_as_probability:1,
> +unused:4;
>   const u8 *user_cookie;
>   u32 user_cookie_len;
>  };
> diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
> index e80637e1d97b..b765f0e81f20 100644
> --- a/include/uapi/linux/psample.h
> +++ b/include/uapi/linux/psample.h
> @@ -8,7 +8,11 @@ enum {
>   PSAMPLE_ATTR_ORIGSIZE,
>   PSAMPLE_ATTR_SAMPLE_GROUP,
>   PSAMPLE_ATTR_GROUP_SEQ,
> - PSAMPLE_ATTR_SAMPLE_RATE,
> + PSAMPLE_ATTR_SAMPLE_RATE,   /* u32, ratio between observed and
> +  * sampled packets or scaled probability
> +  * if PSAMPLE_ATTR_SAMPLE_PROBABILITY
> +  * is set.
> +  */
>   PSAMPLE_ATTR_DATA,
>   PSAMPLE_ATTR_GROUP_REFCOUNT,
>   PSAMPLE_ATTR_TUNNEL,
> @@ -20,6 +24,10 @@ enum {
>   PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
>   PSAMPLE_ATTR_PROTO, /* u16 */
>   PSAMPLE_ATTR_USER_COOKIE,   /* binary, user provided data */
> + PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
> +  * PSAMPLE_ATTR_SAMPLE_RATE as a
> +  * probability scaled 0 - U32_MAX.
> +  */
>  
>   __PSAMPLE_ATTR_MAX
>  };
> diff --git a/include/uapi/linux/tc_act/tc_sample.h 
> b/include/uapi/linux/tc_act/tc_sample.h
> index fee1bcc20793..7ee0735e7b38 100644
> --- a/include/uapi/linux/tc_act/tc_sample.h
> +++ b/include/uapi/linux/tc_act/tc_sample.h
> @@ -18,6 +18,7 @@ enum {
>   TCA_SAMPLE_TRUNC_SIZE,
>   TCA_SAMPLE_PSAMPLE_GROUP,
>   TCA_SAMPLE_PAD,
> + TCA_SAMPLE_PROBABILITY,
>   __TCA_SAMPLE_MAX
>  };
>  #define TCA_SAMPLE_MAX (__TCA_SAMPLE_MAX - 1)

Not a full review yet, but this hunk seems unrelated to the set
as you're not adding rate_as_probability to act_sample.

I suppose, it was part of the plan at some point, but then 

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Ilya Maximets]

On 6/24/24 20:00, Adrián Moreno wrote:
> On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
> [...]
>>
>> Why are we adding IPFIX?  This should work without IPFIX.
>>
>> Having both together can be a separate test.
>>
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=ovs-p0,ip 
>>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
>>> +in_port=ovs-p1,ip 
>>> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
>>
>> It should be possible to wrap these lines a little with dnl.
> 
> I'm curious about wrapping requirements for flows in tests.
> None of them seem to be wrapped, splitting matches and actions won't
> make lines much shorter and having more exotic wrapped & indented
> action arguments seem strage to read (for eyes made to read flows in
> a single line). What wrapping do you have in mind?

No hard requirements, but I find lines that long hard to read
since they are going out of the window width for me and getting
wrapped in random places.


AT_DATA([flows.txt], [dnl
in_port=ovs-p0,ip dnl
actions=dnl
 sample( dnl
  probability=65535, dnl
  collector_set_id=1,dnl
  obs_domain_id=1431655765,  dnl
  obs_point_id=1717986918),  dnl
 output(port=ovs-p1,max_len=100)
...

I agree that above is not very readable...

Or

AT_DATA([flows.txt], [dnl
m4_join([], [
  [in_port=ovs-p0,ip], [ ],
  [actions=],
   
[sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918)],
   [output(port=ovs-p1,max_len=100)]
]
m4_join([], [
  [in_port=ovs-p1,ip], [ ],
  [actions=],
   
[sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377)],
   [output(port=ovs-p0,max_len=100)]
]
)

Or maybe just shorten the lines with a macro:

m4_define([SAMPLE_ACTION],
 
[sample(probability=65535,collector_set_id=$1,obs_domain_id=$2,obs_point_id=$3)]
)

AT_DATA([flows.txt], [dnl
in_port=ovs-p0,ip actions=SAMPLE_ACTION(1, 1431655765, 1717986918), 
output(port=ovs-p1,max_len=100)
in_port=ovs-p1,ip actions=SAMPLE_ACTION(2, 2290649224, 2576980377), 
output(port=ovs-p0,max_len=100)
])

(Haven't tested any of these)

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
[...]
>
> Why are we adding IPFIX?  This should work without IPFIX.
>
> Having both together can be a separate test.
>
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=ovs-p0,ip 
> > actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> > +in_port=ovs-p1,ip 
> > actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
>
> It should be possible to wrap these lines a little with dnl.

I'm curious about wrapping requirements for flows in tests.
None of them seem to be wrapped, splitting matches and actions won't
make lines much shorter and having more exotic wrapped & indented
action arguments seem strage to read (for eyes made to read flows in
a single line). What wrapping do you have in mind?

Thanks.
Adrián

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

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Ilya Maximets]

On 6/24/24 19:38, Adrián Moreno wrote:
> On Mon, Jun 24, 2024 at 04:15:50PM GMT, Ilya Maximets wrote:
>> On 6/24/24 15:14, Adrián Moreno wrote:
>>> On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
 On 6/5/24 22:23, Adrian Moreno wrote:
> Test simultaneous IPFIX and local sampling including slow-path.
>
> Signed-off-by: Adrian Moreno 
> ---
>  tests/system-common-macros.at |   4 ++
>  tests/system-traffic.at   | 105 ++
>  2 files changed, 109 insertions(+)
>

 Not a full review, but see a few comments below.

> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> index 2a68cd664..22b8885e4 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
>  # OVS_CHECK_DROP_ACTION()
>  m4_define([OVS_CHECK_DROP_ACTION],
>  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> ovs-vswitchd.log])])
> +
> +# OVS_CHECK_EMIT_SAMPLE()
> +m4_define([OVS_CHECK_EMIT_SAMPLE],
> +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> ovs-vswitchd.log])])
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index bd7647cbe..babc56b56 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: 
> * * *5002 *2000 *b85e *00
>
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([emit_sample])

 Other tests have a test group name at the beginning.
 You're also adding the test into NSH section.  Add a new section, e.g.,
 'sampling' for this with AT_BANNER.

>>>
>>> Thanks. Will do.
>>>
> +OVS_TRAFFIC_VSWITCHD_START()
> +OVS_CHECK_EMIT_SAMPLE()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], 
> [0], [ignore])
> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], 
> [0], [ignore])

 This looks strange, don't disable IPv6.  Appropriate datapath actions
 should not forward IPv6 traffic, if that is a desired behavior.

>>>
>>> I found tests cleaner if automatic ipv6 traffic is not sent by ports.
>>> But sure, I can deal with that with OpenFlow.
>>
>> Would be good to have two separate tests.  One for IPv4 and one for IPv6.
>>
>>>
 Though, I guess, it would be nice to test capturing both IPv4 and IPv6.

> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> +
> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> +
> +
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> ofproto_dpif_upcall:dbg])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +-- --id=@ipfix create IPFIX 
> targets=\"127.0.0.1:4739\" \
> +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
> ipfix=@ipfix, local_sample_group=10 \
> +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
> ipfix=@ipfix, local_sample_group=12],
> + [0], [ignore])

 Why are we adding IPFIX?  This should work without IPFIX.

>>>
>>> It is intentional. I want to test both features work simulteaneously.
>>
>> Tests should be simple.  At least, we should have at least one very simple
>> test, so if it fails we know that there is an issue in the basic logic.
>>
>> It's valuable to test a combination of the two, but it should be a separate
>> test.  So, 3 tests so far: IPv4, IPv6, psample+ipfix.  Maybe the 4th separate
>> test for the slow path.
>>
> 
> Ack.
> 
>>>
 Having both together can be a separate test.

> +
> +AT_DATA([flows.txt], [dnl
> +in_port=ovs-p0,ip 
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> +in_port=ovs-p1,ip 
> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)

 It should be possible to wrap these lines a little with dnl.

 Also, please make IDs non-symmetric, so we can't check that the
 byte order is correct.

>>>
>>> Originally, I thought just concatenating the values in native endianness
>>> as a kind of replacement for the user_action_cookie (hence the symmetric
>>> values). But I'm having second thoughts. I plan to change it in the next
>>> version and store it in "network" order. I will change the test to
>>> verify it accordingly.
>>
>> I'm not sure I get that, but just in case: OpenFlow messages themselves
>> should be encoded w

Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote:
> On 6/24/24 15:19, Adrián Moreno wrote:
> > On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote:
> >> On 6/5/24 22:23, Adrian Moreno wrote:
> >>> Use the newly added emit_sample to implement OpenFlow sample() actions
> >>> with local sampling configuration.
> >>>
> >>> Signed-off-by: Adrian Moreno 
> >>> ---
> >>>  ofproto/ofproto-dpif-lsample.c |  17 
> >>>  ofproto/ofproto-dpif-lsample.h |   6 ++
> >>>  ofproto/ofproto-dpif-xlate.c   | 163 -
> >>>  ofproto/ofproto-dpif-xlate.h   |   3 +-
> >>>  ofproto/ofproto-dpif.c |   2 +-
> >>>  5 files changed, 144 insertions(+), 47 deletions(-)
> >>
> >> Not a full review, just a note that this patch needs tests in 
> >> ofproto-dpif.at
> >> that would check that we generate correct datapath flows.
> >>
> >
> > I thought about that, but ofproto-dpif.at is based on dummy datapat
> > (userspace) which does not support this action. The configuration will
> > be ignored and the datapath actions will not be generated. That's why I
> > relied on system-traffic.at tests. Do you see any way around this?
>
> We don't need actual datapath support if we're not sending any actual
> trafic.  You should be able to just turn on the capability and check
> that ofproto/trace generates correct actions.
>
> We test kernel tunnels this way, for example.  See the macro
> OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP.  And some other similar checks.
>

OK. Thanks for the pointer. I'll give it a try.

Adrián.

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

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 04:15:50PM GMT, Ilya Maximets wrote:
> On 6/24/24 15:14, Adrián Moreno wrote:
> > On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
> >> On 6/5/24 22:23, Adrian Moreno wrote:
> >>> Test simultaneous IPFIX and local sampling including slow-path.
> >>>
> >>> Signed-off-by: Adrian Moreno 
> >>> ---
> >>>  tests/system-common-macros.at |   4 ++
> >>>  tests/system-traffic.at   | 105 ++
> >>>  2 files changed, 109 insertions(+)
> >>>
> >>
> >> Not a full review, but see a few comments below.
> >>
> >>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> >>> index 2a68cd664..22b8885e4 100644
> >>> --- a/tests/system-common-macros.at
> >>> +++ b/tests/system-common-macros.at
> >>> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
> >>>  # OVS_CHECK_DROP_ACTION()
> >>>  m4_define([OVS_CHECK_DROP_ACTION],
> >>>  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> >>> ovs-vswitchd.log])])
> >>> +
> >>> +# OVS_CHECK_EMIT_SAMPLE()
> >>> +m4_define([OVS_CHECK_EMIT_SAMPLE],
> >>> +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> >>> ovs-vswitchd.log])])
> >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> >>> index bd7647cbe..babc56b56 100644
> >>> --- a/tests/system-traffic.at
> >>> +++ b/tests/system-traffic.at
> >>> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: 
> >>> * * *5002 *2000 *b85e *00
> >>>
> >>>  OVS_TRAFFIC_VSWITCHD_STOP
> >>>  AT_CLEANUP
> >>> +
> >>> +AT_SETUP([emit_sample])
> >>
> >> Other tests have a test group name at the beginning.
> >> You're also adding the test into NSH section.  Add a new section, e.g.,
> >> 'sampling' for this with AT_BANNER.
> >>
> >
> > Thanks. Will do.
> >
> >>> +OVS_TRAFFIC_VSWITCHD_START()
> >>> +OVS_CHECK_EMIT_SAMPLE()
> >>> +
> >>> +ADD_NAMESPACES(at_ns0, at_ns1)
> >>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], 
> >>> [0], [ignore])
> >>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], 
> >>> [0], [ignore])
> >>
> >> This looks strange, don't disable IPv6.  Appropriate datapath actions
> >> should not forward IPv6 traffic, if that is a desired behavior.
> >>
> >
> > I found tests cleaner if automatic ipv6 traffic is not sent by ports.
> > But sure, I can deal with that with OpenFlow.
>
> Would be good to have two separate tests.  One for IPv4 and one for IPv6.
>
> >
> >> Though, I guess, it would be nice to test capturing both IPv4 and IPv6.
> >>
> >>> +
> >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> >>> +
> >>> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> >>> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> >>> +
> >>> +
> >>> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> >>> ofproto_dpif_upcall:dbg])
> >>> +
> >>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> >>> +-- --id=@ipfix create IPFIX 
> >>> targets=\"127.0.0.1:4739\" \
> >>> +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
> >>> ipfix=@ipfix, local_sample_group=10 \
> >>> +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
> >>> ipfix=@ipfix, local_sample_group=12],
> >>> + [0], [ignore])
> >>
> >> Why are we adding IPFIX?  This should work without IPFIX.
> >>
> >
> > It is intentional. I want to test both features work simulteaneously.
>
> Tests should be simple.  At least, we should have at least one very simple
> test, so if it fails we know that there is an issue in the basic logic.
>
> It's valuable to test a combination of the two, but it should be a separate
> test.  So, 3 tests so far: IPv4, IPv6, psample+ipfix.  Maybe the 4th separate
> test for the slow path.
>

Ack.

> >
> >> Having both together can be a separate test.
> >>
> >>> +
> >>> +AT_DATA([flows.txt], [dnl
> >>> +in_port=ovs-p0,ip 
> >>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> >>> +in_port=ovs-p1,ip 
> >>> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
> >>
> >> It should be possible to wrap these lines a little with dnl.
> >>
> >> Also, please make IDs non-symmetric, so we can't check that the
> >> byte order is correct.
> >>
> >
> > Originally, I thought just concatenating the values in native endianness
> > as a kind of replacement for the user_action_cookie (hence the symmetric
> > values). But I'm having second thoughts. I plan to change it in the next
> > version and store it in "network" order. I will change the test to
> > verify it accordingly.
>
> I'm not sure I get that, but just in case: OpenFlow messages themselves
> should be encoded with a network byte order.  When printed to the user,
> they sh

Re: [ovs-dev] [PATCH v2 net-next 0/7] selftests: net: Switch pmtu.sh to use the internal ovs script.

[Aaron Conole]

Aaron Conole  writes:

> Jakub Kicinski  writes:
>
>> On Thu, 20 Jun 2024 08:55:54 -0400 Aaron Conole wrote:
>>> This series enhances the ovs-dpctl utility to provide support for set()
>>> and tunnel() flow specifiers, better ipv6 handling support, and the
>>> ability to add tunnel vports, and LWT interfaces.  Finally, it modifies
>>> the pmtu.sh script to call the ovs-dpctl.py utility rather than the
>>> typical OVS userspace utilities.
>>
>> Thanks for the work! 
>>
>> Looks like the series no longer applies because of other changes
>> to the kernel config. Before it stopped applying we got some runs,
>> here's what I see:
>>
>> https://netdev-3.bots.linux.dev/vmksft-net/results/648440/3-pmtu-sh/stdout
>>
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS vxlan4: PMTU exceptions [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS vxlan4: PMTU exceptions - nexthop objects   [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS vxlan4: PMTU exceptions [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS vxlan4: PMTU exceptions - nexthop objects   [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS vxlan6: PMTU exceptions [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS vxlan6: PMTU exceptions - nexthop objects   [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS vxlan6: PMTU exceptions [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS vxlan6: PMTU exceptions - nexthop objects   [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS geneve4: PMTU exceptions[FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS geneve4: PMTU exceptions - nexthop objects  [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS geneve4: PMTU exceptions[FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS geneve4: PMTU exceptions - nexthop objects  [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS geneve6: PMTU exceptions[FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv4, OVS geneve6: PMTU exceptions - nexthop objects  [FAIL]
>> # Cannot find device "ovs_br0"
>> # TEST: IPv6, OVS geneve6: PMTU exceptions[FAIL]
>> # Cannot find device "ovs_br0"
>>
>> Any idea why? Looks like kernel config did include OVS, perhaps we need
>> explicit modprobe now? I don't see any more details in the logs.
>
> Strange.  I expected that the module should have automatically been
> loaded when attempting to communicate with the OVS genetlink family
> type.  At least, that is how it had been working previously.
>
> I'll spend some time looking into it and resubmit a rebased version.
> Thanks, Jakub!

If the ovs module isn't available, then I see:

#   ovs_bridge not supported
# TEST: IPv4, OVS vxlan4: PMTU exceptions [SKIP]

But if it is available, I haven't been able to reproduce such ovs_br0
setup failure - things work.

My branch is rebased on 568ebdaba6370c03360860f1524f646ddd5ca523

Additionally, the "Cannot find device ..." text comes from an iproute2
utility output.  The only place we actually interact with that is via
the call at pmtu.sh:973:

run_cmd ip link set ovs_br0 up

Maybe it is possible that the link isn't up (could some port memory
allocation or message be delaying it?) yet in the virtual environment.
To confirm, is it possible to run in the constrained environment, but
put a 5s sleep or something?  I will add the following either as a
separate patch (ie 7/8), or I can fold it into 6/7 (and drop Stefano's
ACK waiting for another review):


wait_for_if() {
   if ip link show "$2" >/dev/null 2>&1; then return 0; fi

   for d in `seq 1 30`; do
  sleep 1
  if ip link show "$2" >/dev/null 2>&1; then return 0; fi
   done
   return 1
}


setup_ovs_br_internal || setup_ovs_br_vswitchd || return $ksft_skip
+   wait_for_if "ovs_br0"
run_cmd ip link set ovs_br0 up


Does it make sense or does it seem like I am way off base?

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

[ovs-dev] [PATCH ovn v2 2/2] ci: Move DPDK build into container.

[Ales Musil]

The DPDK was built as extra step in the CI, however it is useful to
have it inside the container prepared. This also helps with
reproduction of failures with DPDK by having the exact version inside
the container already.

Also bump the Ubuntu version for container builds to 24.04 to get
Podman 4. This is required to avoid the tar permissions error:

tar: dpdk-23.11/.ci: Cannot change mode to rwxrwxr-x: Operation not permitted

Signed-off-by: Ales Musil 
Acked-by: Eelco Chaudron 
---
v2: Bump the meson version.
Add ack from Eelco.
---
 .ci/ci.sh|  6 --
 .ci/dpdk-build.sh| 62 --
 .ci/dpdk-prepare.sh  | 11 
 .ci/linux-build.sh   |  7 +-
 .github/workflows/containers.yml |  2 +-
 .github/workflows/test.yml   | 81 +---
 Makefile.am  |  2 -
 utilities/containers/fedora/Dockerfile   |  3 +-
 utilities/containers/prepare.sh  | 56 
 utilities/containers/py-requirements.txt |  1 +
 utilities/containers/ubuntu/Dockerfile   |  3 +-
 11 files changed, 66 insertions(+), 168 deletions(-)
 delete mode 100755 .ci/dpdk-build.sh
 delete mode 100755 .ci/dpdk-prepare.sh

diff --git a/.ci/ci.sh b/.ci/ci.sh
index 6beeace84..f543967dc 100755
--- a/.ci/ci.sh
+++ b/.ci/ci.sh
@@ -16,7 +16,6 @@
 
 OVN_PATH=${OVN_PATH:-$PWD}
 OVS_PATH=${OVS_PATH:-$OVN_PATH/ovs}
-DPDK_PATH=${DPDK_PATH:-$OVN_PATH/dpdk-dir}
 CONTAINER_CMD=${CONTAINER_CMD:-podman}
 CONTAINER_WORKSPACE="/workspace"
 CONTAINER_WORKDIR="/workspace/ovn-tmp"
@@ -163,17 +162,12 @@ if [ "$ARCH" = "aarch64" ] && ! check_clang_version_ge 
"16.0.0"; then
 ASAN_OPTIONS="detect_leaks=0"
 fi
 
-if [ -z "$DPDK" ]; then
-   mkdir -p "$DPDK_PATH"
-fi
-
 CONTAINER_ID="$($CONTAINER_CMD run --privileged -d \
 --pids-limit=-1 \
 --env ASAN_OPTIONS=$ASAN_OPTIONS \
 -v /lib/modules/$(uname -r):/lib/modules/$(uname -r):ro \
 -v $OVN_PATH:$CONTAINER_WORKSPACE/ovn:Z \
 -v $OVS_PATH:$CONTAINER_WORKSPACE/ovs:Z \
--v $DPDK_PATH:$CONTAINER_WORKSPACE/dpdk-dir:Z \
 $IMAGE_NAME)"
 trap remove_container EXIT
 
diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
deleted file mode 100755
index 0c13c98c9..0
--- a/.ci/dpdk-build.sh
+++ /dev/null
@@ -1,62 +0,0 @@
-#!/bin/bash
-
-set -o errexit
-set -x
-
-function build_dpdk()
-{
-local DPDK_VER=$1
-local DPDK_OPTS=""
-local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
-local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
-
-rm -rf dpdk-src
-rm -rf $DPDK_INSTALL_DIR
-
-if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
-git clone --single-branch $DPDK_GIT dpdk-src -b "${DPDK_VER##refs/*/}"
-pushd dpdk-src
-git log -1 --oneline
-else
-wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
-tar xvf dpdk-$1.tar.xz > /dev/null
-DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
-mv ${DIR_NAME} dpdk-src
-pushd dpdk-src
-fi
-
-# Switching to 'default' machine to make the dpdk cache usable on
-# different CPUs. We can't be sure that all CI machines are exactly same.
-DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
-
-# Disable building DPDK unit tests. Not needed for OVS build or tests.
-DPDK_OPTS="$DPDK_OPTS -Dtests=false"
-
-# Disable DPDK developer mode, this results in less build checks and less
-# meson verbose outputs.
-DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
-
-# OVS compilation and the "ovn-system-dpdk" unit tests (run in the CI)
-# only depend on virtio/tap drivers.
-# We can disable all remaining drivers to save compilation time.
-DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
-# OVS depends on the vhost library (and its dependencies).
-# net/tap depends on the gso library.
-DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
-
-# Install DPDK using prefix.
-DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
-
-meson $DPDK_OPTS build
-ninja -C build
-ninja -C build install
-popd
-
-# Remove examples sources.
-rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
-
-echo "Installed DPDK in $DPDK_INSTALL_DIR"
-echo "${DPDK_VER}" > ${VERSION_FILE}
-}
-
-build_dpdk $DPDK_VER
diff --git a/.ci/dpdk-prepare.sh b/.ci/dpdk-prepare.sh
deleted file mode 100755
index 5543da90a..0
--- a/.ci/dpdk-prepare.sh
+++ /dev/null
@@ -1,11 +0,0 @@
-#!/bin/bash
-
-set -ev
-
-# Installing wheel separately because it may be needed to build some
-# of the packages during dependency backtracking and pip >= 22.0 will
-# stop backtracking on build failures:
-# https://github.com/pypa/pip/issues/10655
-pip3 install --disable-pip-version-check --user wheel
-pip3 install --disable-pip-version-check --user pyelftools
-pip3 install --user  'meson==0.53.2'
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index d4c57d

[ovs-dev] [PATCH ovn v2 1/2] ci: Move common build steps into script.

[Ales Musil]

Move common preparation steps into script that can be invoked by both
container builds. This will ensure that any update will be reflected
in both containers, and it reduces the duplication between both
containers. At the same time use the --user argument which avoids
the error below and allows pip to upgrade itself:

ERROR: Cannot uninstall pip 24.0, RECORD file not found.
Hint: The package was installed by debian.

Signed-off-by: Ales Musil 
Acked-by: Eelco Chaudron 
---
v2: Use --user for pip update.
Add ack from Eelco.
---
 utilities/automake.mk  |  1 +
 utilities/containers/fedora/Dockerfile | 35 
 utilities/containers/prepare.sh| 37 ++
 utilities/containers/ubuntu/Dockerfile | 37 +-
 4 files changed, 49 insertions(+), 61 deletions(-)
 create mode 100755 utilities/containers/prepare.sh

diff --git a/utilities/automake.mk b/utilities/automake.mk
index de4f6efb5..03e9096fa 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -42,6 +42,7 @@ EXTRA_DIST += \
 utilities/containers/Makefile \
 utilities/containers/openbfdd.patch \
 utilities/containers/py-requirements.txt \
+utilities/containers/prepare.sh \
 utilities/containers/fedora/Dockerfile \
 utilities/containers/ubuntu/Dockerfile \
 utilities/docker/Makefile \
diff --git a/utilities/containers/fedora/Dockerfile 
b/utilities/containers/fedora/Dockerfile
index 019e9f138..f28c00b5d 100755
--- a/utilities/containers/fedora/Dockerfile
+++ b/utilities/containers/fedora/Dockerfile
@@ -45,41 +45,16 @@ RUN dnf -y update \
 && \
 dnf clean all
 
-# Compile sparse from source
-WORKDIR /workspace/sparse
+ENV PATH "${PATH}:${HOME}/bin:${HOME}/.local/bin"
 
-RUN git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git \
-/workspace/sparse \
-&& \
-make -j4 PREFIX=/usr HAVE_LLVM= HAVE_SQLITE= install
-
-# Compile OpenBFDD from source
-WORKDIR /workspace/OpenBFDD
+WORKDIR /workspace
 
 COPY $CONTAINERS_PATH/openbfdd.patch /tmp/openbfdd.patch
 
-RUN git clone https://github.com/dyninc/OpenBFDD.git \
-/workspace/OpenBFDD \
-&& \
-git apply /tmp/openbfdd.patch \
-&& \
-./autogen.sh \
-&& \
-./configure --enable-silent-rules \
-&& \
-make \
-&& \
-make install
-
-WORKDIR /workspace
-
 COPY $CONTAINERS_PATH/py-requirements.txt /tmp/py-requirements.txt
 
-# Update and install pip dependencies
-RUN python3 -m pip install --upgrade pip \
-&& \
-python3 -m pip install wheel \
-&& \
-python3 -m pip install -r /tmp/py-requirements.txt
+COPY $CONTAINERS_PATH/prepare.sh /tmp/prepare.sh
+
+RUN /tmp/prepare.sh
 
 CMD ["/usr/sbin/init"]
diff --git a/utilities/containers/prepare.sh b/utilities/containers/prepare.sh
new file mode 100755
index 0..b3baa4345
--- /dev/null
+++ b/utilities/containers/prepare.sh
@@ -0,0 +1,37 @@
+#!/bin/bash -xe
+
+function compile_sparse()
+{
+git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git \
+/workspace/sparse
+
+pushd sparse
+make -j4 PREFIX=/usr HAVE_LLVM= HAVE_SQLITE= install
+popd
+}
+
+function compile_openbfdd()
+{
+git clone https://github.com/dyninc/OpenBFDD.git \
+/workspace/OpenBFDD
+
+pushd OpenBFDD
+git apply /tmp/openbfdd.patch
+./autogen.sh
+./configure --enable-silent-rules
+make
+make install
+popd
+}
+
+function install_python_dep()
+{
+# The --user should be removed once pip can be upgraded on Ubuntu.
+python3 -m pip install --user --upgrade pip
+python3 -m pip install wheel
+python3 -m pip install -r /tmp/py-requirements.txt
+}
+
+compile_sparse
+compile_openbfdd
+install_python_dep
diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index ce7ce16c6..49ba861ac 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -45,48 +45,23 @@ RUN apt update -y \
 && \
 apt clean
 
-# Compile sparse from source
-WORKDIR /workspace/sparse
+ENV PATH "${PATH}:${HOME}/bin:${HOME}/.local/bin"
 
-RUN git clone git://git.kernel.org/pub/scm/devel/sparse/sparse.git \
-/workspace/sparse \
-&& \
-make -j4 PREFIX=/usr HAVE_LLVM= HAVE_SQLITE= install
-
-# Compile OpenBFDD from source
-WORKDIR /workspace/OpenBFDD
+WORKDIR /workspace
 
 COPY $CONTAINERS_PATH/openbfdd.patch /tmp/openbfdd.patch
 
-RUN git clone https://github.com/dyninc/OpenBFDD.git \
-/workspace/OpenBFDD \
-&& \
-git apply /tmp/openbfdd.patch \
-&& \
-./autogen.sh \
-&& \
-./configure --enable-silent-rules \
-&& \
-make \
-&& \
-make install
-
-WORKDIR /workspace
-
 COPY $CONTAINERS_PATH/py-requirements.txt /tmp/py-requirements.txt
 
+COPY $CONTAINERS_PATH/prepare.sh /tmp/prepare.sh
+
 # Ubuntu 24.04 marks the Python installation as externally managed, allow pip
 # to install the packages despite that.
 ENV PIP_BREAK_SYSTEM_

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Ilya Maximets]

On 6/24/24 15:14, Adrián Moreno wrote:
> On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
>> On 6/5/24 22:23, Adrian Moreno wrote:
>>> Test simultaneous IPFIX and local sampling including slow-path.
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  tests/system-common-macros.at |   4 ++
>>>  tests/system-traffic.at   | 105 ++
>>>  2 files changed, 109 insertions(+)
>>>
>>
>> Not a full review, but see a few comments below.
>>
>>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
>>> index 2a68cd664..22b8885e4 100644
>>> --- a/tests/system-common-macros.at
>>> +++ b/tests/system-common-macros.at
>>> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
>>>  # OVS_CHECK_DROP_ACTION()
>>>  m4_define([OVS_CHECK_DROP_ACTION],
>>>  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
>>> ovs-vswitchd.log])])
>>> +
>>> +# OVS_CHECK_EMIT_SAMPLE()
>>> +m4_define([OVS_CHECK_EMIT_SAMPLE],
>>> +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
>>> ovs-vswitchd.log])])
>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>>> index bd7647cbe..babc56b56 100644
>>> --- a/tests/system-traffic.at
>>> +++ b/tests/system-traffic.at
>>> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: 
>>> * * *5002 *2000 *b85e *00
>>>
>>>  OVS_TRAFFIC_VSWITCHD_STOP
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([emit_sample])
>>
>> Other tests have a test group name at the beginning.
>> You're also adding the test into NSH section.  Add a new section, e.g.,
>> 'sampling' for this with AT_BANNER.
>>
> 
> Thanks. Will do.
> 
>>> +OVS_TRAFFIC_VSWITCHD_START()
>>> +OVS_CHECK_EMIT_SAMPLE()
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1)
>>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>>> [ignore])
>>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>>> [ignore])
>>
>> This looks strange, don't disable IPv6.  Appropriate datapath actions
>> should not forward IPv6 traffic, if that is a desired behavior.
>>
> 
> I found tests cleaner if automatic ipv6 traffic is not sent by ports.
> But sure, I can deal with that with OpenFlow.

Would be good to have two separate tests.  One for IPv4 and one for IPv6.

> 
>> Though, I guess, it would be nice to test capturing both IPv4 and IPv6.
>>
>>> +
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
>>> +
>>> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
>>> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
>>> +
>>> +
>>> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
>>> ofproto_dpif_upcall:dbg])
>>> +
>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
>>> +-- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" 
>>> \
>>> +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
>>> ipfix=@ipfix, local_sample_group=10 \
>>> +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
>>> ipfix=@ipfix, local_sample_group=12],
>>> + [0], [ignore])
>>
>> Why are we adding IPFIX?  This should work without IPFIX.
>>
> 
> It is intentional. I want to test both features work simulteaneously.

Tests should be simple.  At least, we should have at least one very simple
test, so if it fails we know that there is an issue in the basic logic.

It's valuable to test a combination of the two, but it should be a separate
test.  So, 3 tests so far: IPv4, IPv6, psample+ipfix.  Maybe the 4th separate
test for the slow path.

> 
>> Having both together can be a separate test.
>>
>>> +
>>> +AT_DATA([flows.txt], [dnl
>>> +in_port=ovs-p0,ip 
>>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
>>> +in_port=ovs-p1,ip 
>>> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
>>
>> It should be possible to wrap these lines a little with dnl.
>>
>> Also, please make IDs non-symmetric, so we can't check that the
>> byte order is correct.
>>
> 
> Originally, I thought just concatenating the values in native endianness
> as a kind of replacement for the user_action_cookie (hence the symmetric
> values). But I'm having second thoughts. I plan to change it in the next
> version and store it in "network" order. I will change the test to
> verify it accordingly.

I'm not sure I get that, but just in case: OpenFlow messages themselves
should be encoded with a network byte order.  When printed to the user,
they should be printed in a host byte order.  Netlink messages typically
have values in the host byte order, but there are some weird netlink
messages that expect network byte order.  In any case, the psample test
utility should print out values in the host byte order.

> 
>>> +])
>>> +
>>> +AT_CHECK([ovs-ofctl

Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.

[Ilya Maximets]

On 6/24/24 15:19, Adrián Moreno wrote:
> On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote:
>> On 6/5/24 22:23, Adrian Moreno wrote:
>>> Use the newly added emit_sample to implement OpenFlow sample() actions
>>> with local sampling configuration.
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  ofproto/ofproto-dpif-lsample.c |  17 
>>>  ofproto/ofproto-dpif-lsample.h |   6 ++
>>>  ofproto/ofproto-dpif-xlate.c   | 163 -
>>>  ofproto/ofproto-dpif-xlate.h   |   3 +-
>>>  ofproto/ofproto-dpif.c |   2 +-
>>>  5 files changed, 144 insertions(+), 47 deletions(-)
>>
>> Not a full review, just a note that this patch needs tests in ofproto-dpif.at
>> that would check that we generate correct datapath flows.
>>
> 
> I thought about that, but ofproto-dpif.at is based on dummy datapat
> (userspace) which does not support this action. The configuration will
> be ignored and the datapath actions will not be generated. That's why I
> relied on system-traffic.at tests. Do you see any way around this?

We don't need actual datapath support if we're not sending any actual
trafic.  You should be able to just turn on the capability and check
that ofproto/trace generates correct actions.

We test kernel tunnels this way, for example.  See the macro
OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP.  And some other similar checks.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 2/2] ci: Move DPDK build into container.

[Eelco Chaudron]

On 24 Jun 2024, at 15:52, Ales Musil wrote:

> On Mon, Jun 24, 2024 at 1:59 PM Eelco Chaudron  wrote:
>
>>
>>
>> On 20 Jun 2024, at 14:57, Ales Musil wrote:
>>
>>> The DPDK was built as extra step in the CI, however it is useful to
>>> have it inside the container prepared. This also helps with
>>> reproduction of failures with DPDK by having the exact version inside
>>> the container already.
>>>
>>> Also bump the Ubuntu version for container builds to 24.04 to get
>>> Podman 4. This is required to avoid the tar permissions error:
>>>
>>> tar: dpdk-23.11/.ci: Cannot change mode to rwxrwxr-x: Operation not
>> permitted
>>>
>>> Signed-off-by: Ales Musil 
>>
>> Hi Ales,
>>
>> The change in general looks good. Two small nit comments, but anyway I
>> think the patch is good.
>>
>> Acked-by: Eelco Chaudron 
>>
>> //Eelco
>>
>>> ---
>>>  .ci/ci.sh|  6 --
>>>  .ci/dpdk-build.sh| 62 --
>>>  .ci/dpdk-prepare.sh  | 11 
>>>  .ci/linux-build.sh   |  7 +-
>>>  .github/workflows/containers.yml |  2 +-
>>>  .github/workflows/test.yml   | 81 +---
>>>  Makefile.am  |  2 -
>>>  utilities/containers/fedora/Dockerfile   |  1 +
>>>  utilities/containers/prepare.sh  | 56 
>>>  utilities/containers/py-requirements.txt |  1 +
>>>  utilities/containers/ubuntu/Dockerfile   |  1 +
>>>  11 files changed, 66 insertions(+), 164 deletions(-)
>>>  delete mode 100755 .ci/dpdk-build.sh
>>>  delete mode 100755 .ci/dpdk-prepare.sh
>>>
>>> diff --git a/.ci/ci.sh b/.ci/ci.sh
>>> index 6beeace84..f543967dc 100755
>>> --- a/.ci/ci.sh
>>> +++ b/.ci/ci.sh
>>> @@ -16,7 +16,6 @@
>>>
>>>  OVN_PATH=${OVN_PATH:-$PWD}
>>>  OVS_PATH=${OVS_PATH:-$OVN_PATH/ovs}
>>> -DPDK_PATH=${DPDK_PATH:-$OVN_PATH/dpdk-dir}
>>>  CONTAINER_CMD=${CONTAINER_CMD:-podman}
>>>  CONTAINER_WORKSPACE="/workspace"
>>>  CONTAINER_WORKDIR="/workspace/ovn-tmp"
>>> @@ -163,17 +162,12 @@ if [ "$ARCH" = "aarch64" ] && !
>> check_clang_version_ge "16.0.0"; then
>>>  ASAN_OPTIONS="detect_leaks=0"
>>>  fi
>>>
>>> -if [ -z "$DPDK" ]; then
>>> -   mkdir -p "$DPDK_PATH"
>>> -fi
>>> -
>>>  CONTAINER_ID="$($CONTAINER_CMD run --privileged -d \
>>>  --pids-limit=-1 \
>>>  --env ASAN_OPTIONS=$ASAN_OPTIONS \
>>>  -v /lib/modules/$(uname -r):/lib/modules/$(uname -r):ro \
>>>  -v $OVN_PATH:$CONTAINER_WORKSPACE/ovn:Z \
>>>  -v $OVS_PATH:$CONTAINER_WORKSPACE/ovs:Z \
>>> --v $DPDK_PATH:$CONTAINER_WORKSPACE/dpdk-dir:Z \
>>>  $IMAGE_NAME)"
>>>  trap remove_container EXIT
>>>
>>> diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
>>> deleted file mode 100755
>>> index 0c13c98c9..0
>>> --- a/.ci/dpdk-build.sh
>>> +++ /dev/null
>>> @@ -1,62 +0,0 @@
>>> -#!/bin/bash
>>> -
>>> -set -o errexit
>>> -set -x
>>> -
>>> -function build_dpdk()
>>> -{
>>> -local DPDK_VER=$1
>>> -local DPDK_OPTS=""
>>> -local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
>>> -local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
>>> -
>>> -rm -rf dpdk-src
>>> -rm -rf $DPDK_INSTALL_DIR
>>> -
>>> -if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
>>> -git clone --single-branch $DPDK_GIT dpdk-src -b
>> "${DPDK_VER##refs/*/}"
>>> -pushd dpdk-src
>>> -git log -1 --oneline
>>> -else
>>> -wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
>>> -tar xvf dpdk-$1.tar.xz > /dev/null
>>> -DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
>>> -mv ${DIR_NAME} dpdk-src
>>> -pushd dpdk-src
>>> -fi
>>> -
>>> -# Switching to 'default' machine to make the dpdk cache usable on
>>> -# different CPUs. We can't be sure that all CI machines are exactly
>> same.
>>> -DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
>>> -
>>> -# Disable building DPDK unit tests. Not needed for OVS build or
>> tests.
>>> -DPDK_OPTS="$DPDK_OPTS -Dtests=false"
>>> -
>>> -# Disable DPDK developer mode, this results in less build checks
>> and less
>>> -# meson verbose outputs.
>>> -DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
>>> -
>>> -# OVS compilation and the "ovn-system-dpdk" unit tests (run in the
>> CI)
>>> -# only depend on virtio/tap drivers.
>>> -# We can disable all remaining drivers to save compilation time.
>>> -DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
>>> -# OVS depends on the vhost library (and its dependencies).
>>> -# net/tap depends on the gso library.
>>> -DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
>>> -
>>> -# Install DPDK using prefix.
>>> -DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
>>> -
>>> -meson $DPDK_OPTS build
>>> -ninja -C build
>>> -ninja -C build install
>>> -popd
>>> -
>>> -# Remove examples sources.
>>> -rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
>>> -
>>> -echo "In

Re: [ovs-dev] [PATCH ovn 2/2] ci: Move DPDK build into container.

[Ales Musil]

On Mon, Jun 24, 2024 at 1:59 PM Eelco Chaudron  wrote:

>
>
> On 20 Jun 2024, at 14:57, Ales Musil wrote:
>
> > The DPDK was built as extra step in the CI, however it is useful to
> > have it inside the container prepared. This also helps with
> > reproduction of failures with DPDK by having the exact version inside
> > the container already.
> >
> > Also bump the Ubuntu version for container builds to 24.04 to get
> > Podman 4. This is required to avoid the tar permissions error:
> >
> > tar: dpdk-23.11/.ci: Cannot change mode to rwxrwxr-x: Operation not
> permitted
> >
> > Signed-off-by: Ales Musil 
>
> Hi Ales,
>
> The change in general looks good. Two small nit comments, but anyway I
> think the patch is good.
>
> Acked-by: Eelco Chaudron 
>
> //Eelco
>
> > ---
> >  .ci/ci.sh|  6 --
> >  .ci/dpdk-build.sh| 62 --
> >  .ci/dpdk-prepare.sh  | 11 
> >  .ci/linux-build.sh   |  7 +-
> >  .github/workflows/containers.yml |  2 +-
> >  .github/workflows/test.yml   | 81 +---
> >  Makefile.am  |  2 -
> >  utilities/containers/fedora/Dockerfile   |  1 +
> >  utilities/containers/prepare.sh  | 56 
> >  utilities/containers/py-requirements.txt |  1 +
> >  utilities/containers/ubuntu/Dockerfile   |  1 +
> >  11 files changed, 66 insertions(+), 164 deletions(-)
> >  delete mode 100755 .ci/dpdk-build.sh
> >  delete mode 100755 .ci/dpdk-prepare.sh
> >
> > diff --git a/.ci/ci.sh b/.ci/ci.sh
> > index 6beeace84..f543967dc 100755
> > --- a/.ci/ci.sh
> > +++ b/.ci/ci.sh
> > @@ -16,7 +16,6 @@
> >
> >  OVN_PATH=${OVN_PATH:-$PWD}
> >  OVS_PATH=${OVS_PATH:-$OVN_PATH/ovs}
> > -DPDK_PATH=${DPDK_PATH:-$OVN_PATH/dpdk-dir}
> >  CONTAINER_CMD=${CONTAINER_CMD:-podman}
> >  CONTAINER_WORKSPACE="/workspace"
> >  CONTAINER_WORKDIR="/workspace/ovn-tmp"
> > @@ -163,17 +162,12 @@ if [ "$ARCH" = "aarch64" ] && !
> check_clang_version_ge "16.0.0"; then
> >  ASAN_OPTIONS="detect_leaks=0"
> >  fi
> >
> > -if [ -z "$DPDK" ]; then
> > -   mkdir -p "$DPDK_PATH"
> > -fi
> > -
> >  CONTAINER_ID="$($CONTAINER_CMD run --privileged -d \
> >  --pids-limit=-1 \
> >  --env ASAN_OPTIONS=$ASAN_OPTIONS \
> >  -v /lib/modules/$(uname -r):/lib/modules/$(uname -r):ro \
> >  -v $OVN_PATH:$CONTAINER_WORKSPACE/ovn:Z \
> >  -v $OVS_PATH:$CONTAINER_WORKSPACE/ovs:Z \
> > --v $DPDK_PATH:$CONTAINER_WORKSPACE/dpdk-dir:Z \
> >  $IMAGE_NAME)"
> >  trap remove_container EXIT
> >
> > diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
> > deleted file mode 100755
> > index 0c13c98c9..0
> > --- a/.ci/dpdk-build.sh
> > +++ /dev/null
> > @@ -1,62 +0,0 @@
> > -#!/bin/bash
> > -
> > -set -o errexit
> > -set -x
> > -
> > -function build_dpdk()
> > -{
> > -local DPDK_VER=$1
> > -local DPDK_OPTS=""
> > -local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> > -local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
> > -
> > -rm -rf dpdk-src
> > -rm -rf $DPDK_INSTALL_DIR
> > -
> > -if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
> > -git clone --single-branch $DPDK_GIT dpdk-src -b
> "${DPDK_VER##refs/*/}"
> > -pushd dpdk-src
> > -git log -1 --oneline
> > -else
> > -wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
> > -tar xvf dpdk-$1.tar.xz > /dev/null
> > -DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
> > -mv ${DIR_NAME} dpdk-src
> > -pushd dpdk-src
> > -fi
> > -
> > -# Switching to 'default' machine to make the dpdk cache usable on
> > -# different CPUs. We can't be sure that all CI machines are exactly
> same.
> > -DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
> > -
> > -# Disable building DPDK unit tests. Not needed for OVS build or
> tests.
> > -DPDK_OPTS="$DPDK_OPTS -Dtests=false"
> > -
> > -# Disable DPDK developer mode, this results in less build checks
> and less
> > -# meson verbose outputs.
> > -DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
> > -
> > -# OVS compilation and the "ovn-system-dpdk" unit tests (run in the
> CI)
> > -# only depend on virtio/tap drivers.
> > -# We can disable all remaining drivers to save compilation time.
> > -DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
> > -# OVS depends on the vhost library (and its dependencies).
> > -# net/tap depends on the gso library.
> > -DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
> > -
> > -# Install DPDK using prefix.
> > -DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
> > -
> > -meson $DPDK_OPTS build
> > -ninja -C build
> > -ninja -C build install
> > -popd
> > -
> > -# Remove examples sources.
> > -rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
> > -
> > -echo "Installed DPDK in $DPDK_INSTALL_DIR"
> > -echo "${DPDK_VER}" > ${VERS

Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote:
> On 6/5/24 22:23, Adrian Moreno wrote:
> > Use the newly added emit_sample to implement OpenFlow sample() actions
> > with local sampling configuration.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
> >  ofproto/ofproto-dpif-lsample.c |  17 
> >  ofproto/ofproto-dpif-lsample.h |   6 ++
> >  ofproto/ofproto-dpif-xlate.c   | 163 -
> >  ofproto/ofproto-dpif-xlate.h   |   3 +-
> >  ofproto/ofproto-dpif.c |   2 +-
> >  5 files changed, 144 insertions(+), 47 deletions(-)
>
> Not a full review, just a note that this patch needs tests in ofproto-dpif.at
> that would check that we generate correct datapath flows.
>

I thought about that, but ofproto-dpif.at is based on dummy datapat
(userspace) which does not support this action. The configuration will
be ignored and the datapath actions will not be generated. That's why I
relied on system-traffic.at tests. Do you see any way around this?

Thanks.
Adrián

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

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote:
> On 6/5/24 22:23, Adrian Moreno wrote:
> > Test simultaneous IPFIX and local sampling including slow-path.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
> >  tests/system-common-macros.at |   4 ++
> >  tests/system-traffic.at   | 105 ++
> >  2 files changed, 109 insertions(+)
> >
>
> Not a full review, but see a few comments below.
>
> > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> > index 2a68cd664..22b8885e4 100644
> > --- a/tests/system-common-macros.at
> > +++ b/tests/system-common-macros.at
> > @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
> >  # OVS_CHECK_DROP_ACTION()
> >  m4_define([OVS_CHECK_DROP_ACTION],
> >  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> > ovs-vswitchd.log])])
> > +
> > +# OVS_CHECK_EMIT_SAMPLE()
> > +m4_define([OVS_CHECK_EMIT_SAMPLE],
> > +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> > ovs-vswitchd.log])])
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index bd7647cbe..babc56b56 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: 
> > * * *5002 *2000 *b85e *00
> >
> >  OVS_TRAFFIC_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([emit_sample])
>
> Other tests have a test group name at the beginning.
> You're also adding the test into NSH section.  Add a new section, e.g.,
> 'sampling' for this with AT_BANNER.
>

Thanks. Will do.

> > +OVS_TRAFFIC_VSWITCHD_START()
> > +OVS_CHECK_EMIT_SAMPLE()
> > +
> > +ADD_NAMESPACES(at_ns0, at_ns1)
> > +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> > [ignore])
> > +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> > [ignore])
>
> This looks strange, don't disable IPv6.  Appropriate datapath actions
> should not forward IPv6 traffic, if that is a desired behavior.
>

I found tests cleaner if automatic ipv6 traffic is not sent by ports.
But sure, I can deal with that with OpenFlow.

> Though, I guess, it would be nice to test capturing both IPv4 and IPv6.
>
> > +
> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> > +
> > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> > +
> > +
> > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> > ofproto_dpif_upcall:dbg])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +-- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" 
> > \
> > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
> > ipfix=@ipfix, local_sample_group=10 \
> > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
> > ipfix=@ipfix, local_sample_group=12],
> > + [0], [ignore])
>
> Why are we adding IPFIX?  This should work without IPFIX.
>

It is intentional. I want to test both features work simulteaneously.

> Having both together can be a separate test.
>
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=ovs-p0,ip 
> > actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> > +in_port=ovs-p1,ip 
> > actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
>
> It should be possible to wrap these lines a little with dnl.
>
> Also, please make IDs non-symmetric, so we can't check that the
> byte order is correct.
>

Originally, I thought just concatenating the values in native endianness
as a kind of replacement for the user_action_cookie (hence the symmetric
values). But I'm having second thoughts. I plan to change it in the next
version and store it in "network" order. I will change the test to
verify it accordingly.

> > +])
> > +
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +
> > +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid])
>
> Should we wait for the test app to start listening?  i.e. there might be
> a race between the listener and the ping below.
>

I guess so.

> > +
> > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> > +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> > +])
>
> We may need to add -W to the ping.
>

Ack.

> > +
> > +m4_define([SAMPLE1], [group_id=0xa 
> > obs_domain=0x,obs_point=0x 
> > .*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])
> > +m4_define([SAMPLE2], [group_id=0xc 
> > obs_domain=0x,obs_point=0x 
> > .*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])
>
> May use m4_join to wrap these lines.
>
> > +AT_CHECK([grep -E 'SAMPLE1' psample.out >/dev/null])
> > +AT_CHECK([grep -E 'SAMPLE2' psample.out >/dev/null])
>
> 'grep -q' to avoid the outpu

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Adrián Moreno]

On Mon, Jun 24, 2024 at 01:23:55PM GMT, Eelco Chaudron wrote:
>
>
> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>
> > Test simultaneous IPFIX and local sampling including slow-path.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
> >  tests/system-common-macros.at |   4 ++
> >  tests/system-traffic.at   | 105 ++
> >  2 files changed, 109 insertions(+)
> >
> > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> > index 2a68cd664..22b8885e4 100644
> > --- a/tests/system-common-macros.at
> > +++ b/tests/system-common-macros.at
> > @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
> >  # OVS_CHECK_DROP_ACTION()
> >  m4_define([OVS_CHECK_DROP_ACTION],
> >  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> > ovs-vswitchd.log])])
> > +
> > +# OVS_CHECK_EMIT_SAMPLE()
> > +m4_define([OVS_CHECK_EMIT_SAMPLE],
> > +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> > ovs-vswitchd.log])])
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index bd7647cbe..babc56b56 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: 
> > * * *5002 *2000 *b85e *00
> >
> >  OVS_TRAFFIC_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([emit_sample])
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +OVS_CHECK_EMIT_SAMPLE()
> > +
> > +ADD_NAMESPACES(at_ns0, at_ns1)
> > +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> > [ignore])
> > +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> > [ignore])
> > +
> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> > +
> > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> > +
> > +
> > +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> > ofproto_dpif_upcall:dbg])
> > +
> > +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> > +-- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" 
> > \
> > +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
> > ipfix=@ipfix, local_sample_group=10 \
> > +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
> > ipfix=@ipfix, local_sample_group=12],
> > + [0], [ignore])
> > +
> > +AT_DATA([flows.txt], [dnl
> > +in_port=ovs-p0,ip 
> > actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> > +in_port=ovs-p1,ip 
> > actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
> > +])
> > +
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +
> > +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid])
> > +
> > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> > +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> > +])
> > +
> > +m4_define([SAMPLE1], [group_id=0xa 
> > obs_domain=0x,obs_point=0x 
> > .*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])
> > +m4_define([SAMPLE2], [group_id=0xc 
> > obs_domain=0x,obs_point=0x 
> > .*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])
> > +AT_CHECK([grep -E 'SAMPLE1' psample.out >/dev/null])
> > +AT_CHECK([grep -E 'SAMPLE2' psample.out >/dev/null])
> > +
> > +AT_CHECK([ovs-ofctl dump-ipfix-flow br0 | sed 's/tx pkts=[[0-9]]*/tx 
> > pkts=24/' | sed 's/tx errs=[[0-9]]*/tx errs=0/'], [0], [dnl
> > +NXST_IPFIX_FLOW reply (xid=0x2): 2 ids
> > +  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, 
> > tx pkts=24
> > +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> > +  id   2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, 
> > tx pkts=24
> > +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> > +])
>
> This is not a review, just a heads up so you are aware that this test fails 
> sometimes:
>
> +++ 
> /home/echaudron/Documents/review/ovs_adrian_psample/tests/system-kmod-testsuite.dir/at-groups/178/stdout
>   2024-06-24 13:05:35.05700 +0200
> @@ -1,6 +1,6 @@
>  NXST_IPFIX_FLOW reply (xid=0x2): 2 ids
> -  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
> -  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
>id   2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
>pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> +  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
> +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
>
> The order does not seem to be fixed all the time (same for the one below).
>

Ugh, I assumed the output would be ordered. I'll deal with it manually
in the test.

> //Eelco
>
> > +
> > +AT_CHECK([ovs-

Re: [ovs-dev] [PATCH v4] netdev-dpdk: Use LSC interrupt mode.

[Kevin Traynor]

On 19/06/2024 17:00, David Marchand wrote:
> Querying link status may get delayed for an undeterministic (long) time
> with mlx5 ports. This is a consequence of the mlx5 driver calling ethtool
> kernel API and getting stuck on the kernel RTNL lock while some other
> operation is in progress under this lock.
> 
> One impact for long link status query is that it is called under the bond
> lock taken in write mode periodically in bond_run().
> In parallel, datapath threads may block requesting to read bonding related
> info (like for example in bond_check_admissibility()).
> 
> The LSC interrupt mode is available with many DPDK drivers and is used by
> default with testpmd.
> 
> It seems safe enough to switch on this feature by default in OVS.
> We keep the per interface option to disable this feature in case of an
> unforeseen bug.
> 
> Signed-off-by: David Marchand 
> Reviewed-by: Robin Jarry 
> Acked-by: Mike Pattrick 
> ---
> Changes since v3:
> - updated logging in case of error,
> 
> Changes since v2:
> - fixed typo in NEWS,
> 
> Changes since v1:
> - (early) fail when interrupt lsc is requested by user but not supported
>   by the driver,
> - otherwise, log a debug message if user did not request interrupt mode,
> 
> ---
>  Documentation/topics/dpdk/phy.rst |  4 ++--
>  NEWS  |  3 +++
>  lib/netdev-dpdk.c | 13 -
>  vswitchd/vswitch.xml  |  8 
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 

Applied to main branch. Thanks David, Robin, Ilya, Mike, Maxime and Aaron.

It seems this is a popular patch :-)

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

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Ilya Maximets]

On 6/5/24 22:23, Adrian Moreno wrote:
> Test simultaneous IPFIX and local sampling including slow-path.
> 
> Signed-off-by: Adrian Moreno 
> ---
>  tests/system-common-macros.at |   4 ++
>  tests/system-traffic.at   | 105 ++
>  2 files changed, 109 insertions(+)
> 

Not a full review, but see a few comments below.

> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> index 2a68cd664..22b8885e4 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
>  # OVS_CHECK_DROP_ACTION()
>  m4_define([OVS_CHECK_DROP_ACTION],
>  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> ovs-vswitchd.log])])
> +
> +# OVS_CHECK_EMIT_SAMPLE()
> +m4_define([OVS_CHECK_EMIT_SAMPLE],
> +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> ovs-vswitchd.log])])
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index bd7647cbe..babc56b56 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: * 
> * *5002 *2000 *b85e *00
>  
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([emit_sample])

Other tests have a test group name at the beginning.
You're also adding the test into NSH section.  Add a new section, e.g.,
'sampling' for this with AT_BANNER.

> +OVS_TRAFFIC_VSWITCHD_START()
> +OVS_CHECK_EMIT_SAMPLE()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> [ignore])
> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> [ignore])

This looks strange, don't disable IPv6.  Appropriate datapath actions
should not forward IPv6 traffic, if that is a desired behavior.

Though, I guess, it would be nice to test capturing both IPv4 and IPv6.

> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> +
> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> +
> +
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> ofproto_dpif_upcall:dbg])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +-- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \
> +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
> ipfix=@ipfix, local_sample_group=10 \
> +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
> ipfix=@ipfix, local_sample_group=12],
> + [0], [ignore])

Why are we adding IPFIX?  This should work without IPFIX.

Having both together can be a separate test.

> +
> +AT_DATA([flows.txt], [dnl
> +in_port=ovs-p0,ip 
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> +in_port=ovs-p1,ip 
> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)

It should be possible to wrap these lines a little with dnl.

Also, please make IDs non-symmetric, so we can't check that the
byte order is correct.

> +])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid])

Should we wait for the test app to start listening?  i.e. there might be
a race between the listener and the ping below.

> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> +])

We may need to add -W to the ping.

> +
> +m4_define([SAMPLE1], [group_id=0xa 
> obs_domain=0x,obs_point=0x 
> .*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])
> +m4_define([SAMPLE2], [group_id=0xc 
> obs_domain=0x,obs_point=0x 
> .*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])

May use m4_join to wrap these lines.

> +AT_CHECK([grep -E 'SAMPLE1' psample.out >/dev/null])
> +AT_CHECK([grep -E 'SAMPLE2' psample.out >/dev/null])

'grep -q' to avoid the output redirection.  We may need to wait for them also,
i.e. OVS_WAIT_UNTIL.

> +
> +AT_CHECK([ovs-ofctl dump-ipfix-flow br0 | sed 's/tx pkts=[[0-9]]*/tx 
> pkts=24/' | sed 's/tx errs=[[0-9]]*/tx errs=0/'], [0],

Wrap the line with '\'.

 [dnl
> +NXST_IPFIX_FLOW reply (xid=0x2): 2 ids
> +  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
> +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> +  id   2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
> +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> +])
> +
> +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl
> +local sample statistics for bridge "br0":
> +- Collector Set ID: 1
> +  Local Sample Group: 10
> +  Total number of bytes: 98
> +  Total number of packets: 1
> +
> +- Colle

Re: [ovs-dev] [PATCH ovn 2/2] ci: Move DPDK build into container.

[Eelco Chaudron]

On 20 Jun 2024, at 14:57, Ales Musil wrote:

> The DPDK was built as extra step in the CI, however it is useful to
> have it inside the container prepared. This also helps with
> reproduction of failures with DPDK by having the exact version inside
> the container already.
>
> Also bump the Ubuntu version for container builds to 24.04 to get
> Podman 4. This is required to avoid the tar permissions error:
>
> tar: dpdk-23.11/.ci: Cannot change mode to rwxrwxr-x: Operation not permitted
>
> Signed-off-by: Ales Musil 

Hi Ales,

The change in general looks good. Two small nit comments, but anyway I think 
the patch is good.

Acked-by: Eelco Chaudron 

//Eelco

> ---
>  .ci/ci.sh|  6 --
>  .ci/dpdk-build.sh| 62 --
>  .ci/dpdk-prepare.sh  | 11 
>  .ci/linux-build.sh   |  7 +-
>  .github/workflows/containers.yml |  2 +-
>  .github/workflows/test.yml   | 81 +---
>  Makefile.am  |  2 -
>  utilities/containers/fedora/Dockerfile   |  1 +
>  utilities/containers/prepare.sh  | 56 
>  utilities/containers/py-requirements.txt |  1 +
>  utilities/containers/ubuntu/Dockerfile   |  1 +
>  11 files changed, 66 insertions(+), 164 deletions(-)
>  delete mode 100755 .ci/dpdk-build.sh
>  delete mode 100755 .ci/dpdk-prepare.sh
>
> diff --git a/.ci/ci.sh b/.ci/ci.sh
> index 6beeace84..f543967dc 100755
> --- a/.ci/ci.sh
> +++ b/.ci/ci.sh
> @@ -16,7 +16,6 @@
>
>  OVN_PATH=${OVN_PATH:-$PWD}
>  OVS_PATH=${OVS_PATH:-$OVN_PATH/ovs}
> -DPDK_PATH=${DPDK_PATH:-$OVN_PATH/dpdk-dir}
>  CONTAINER_CMD=${CONTAINER_CMD:-podman}
>  CONTAINER_WORKSPACE="/workspace"
>  CONTAINER_WORKDIR="/workspace/ovn-tmp"
> @@ -163,17 +162,12 @@ if [ "$ARCH" = "aarch64" ] && ! check_clang_version_ge 
> "16.0.0"; then
>  ASAN_OPTIONS="detect_leaks=0"
>  fi
>
> -if [ -z "$DPDK" ]; then
> -   mkdir -p "$DPDK_PATH"
> -fi
> -
>  CONTAINER_ID="$($CONTAINER_CMD run --privileged -d \
>  --pids-limit=-1 \
>  --env ASAN_OPTIONS=$ASAN_OPTIONS \
>  -v /lib/modules/$(uname -r):/lib/modules/$(uname -r):ro \
>  -v $OVN_PATH:$CONTAINER_WORKSPACE/ovn:Z \
>  -v $OVS_PATH:$CONTAINER_WORKSPACE/ovs:Z \
> --v $DPDK_PATH:$CONTAINER_WORKSPACE/dpdk-dir:Z \
>  $IMAGE_NAME)"
>  trap remove_container EXIT
>
> diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
> deleted file mode 100755
> index 0c13c98c9..0
> --- a/.ci/dpdk-build.sh
> +++ /dev/null
> @@ -1,62 +0,0 @@
> -#!/bin/bash
> -
> -set -o errexit
> -set -x
> -
> -function build_dpdk()
> -{
> -local DPDK_VER=$1
> -local DPDK_OPTS=""
> -local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> -local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
> -
> -rm -rf dpdk-src
> -rm -rf $DPDK_INSTALL_DIR
> -
> -if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
> -git clone --single-branch $DPDK_GIT dpdk-src -b 
> "${DPDK_VER##refs/*/}"
> -pushd dpdk-src
> -git log -1 --oneline
> -else
> -wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
> -tar xvf dpdk-$1.tar.xz > /dev/null
> -DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
> -mv ${DIR_NAME} dpdk-src
> -pushd dpdk-src
> -fi
> -
> -# Switching to 'default' machine to make the dpdk cache usable on
> -# different CPUs. We can't be sure that all CI machines are exactly same.
> -DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
> -
> -# Disable building DPDK unit tests. Not needed for OVS build or tests.
> -DPDK_OPTS="$DPDK_OPTS -Dtests=false"
> -
> -# Disable DPDK developer mode, this results in less build checks and less
> -# meson verbose outputs.
> -DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
> -
> -# OVS compilation and the "ovn-system-dpdk" unit tests (run in the CI)
> -# only depend on virtio/tap drivers.
> -# We can disable all remaining drivers to save compilation time.
> -DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
> -# OVS depends on the vhost library (and its dependencies).
> -# net/tap depends on the gso library.
> -DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
> -
> -# Install DPDK using prefix.
> -DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
> -
> -meson $DPDK_OPTS build
> -ninja -C build
> -ninja -C build install
> -popd
> -
> -# Remove examples sources.
> -rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
> -
> -echo "Installed DPDK in $DPDK_INSTALL_DIR"
> -echo "${DPDK_VER}" > ${VERSION_FILE}
> -}
> -
> -build_dpdk $DPDK_VER
> diff --git a/.ci/dpdk-prepare.sh b/.ci/dpdk-prepare.sh
> deleted file mode 100755
> index 5543da90a..0
> --- a/.ci/dpdk-prepare.sh
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -#!/bin/bash
> -
> -set -ev
> -
> -# Installing wheel separately because it may be needed to build some
> 

Re: [ovs-dev] [PATCH ovn 1/2] ci: Move common build steps into script.

[Eelco Chaudron]

On 20 Jun 2024, at 14:57, Ales Musil wrote:

> Move common preparation steps into script that can be invoked by both
> container builds. This will ensure that any update will be reflected
> in both containers, and it reduces the duplication between both
> containers.
>
> Signed-off-by: Ales Musil 

Hi Ales, The changes look good to me. Only did a visual (GitHub action) review, 
so no local experimentation.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2] Userspace: Software fallback for UDP encapsulated TCP segmentation.

[David Marchand]

Some additional comments.

On Thu, Jun 13, 2024 at 4:54 AM Mike Pattrick  wrote:
> @@ -89,14 +96,19 @@ dp_packet_gso(struct dp_packet *p, struct dp_packet_batch 
> **batches)
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>  struct dp_packet_batch *curr_batch = *batches;
>  struct tcp_header *tcp_hdr;
> +struct udp_header *tnl_hdr;
>  struct ip_header *ip_hdr;
> +uint16_t inner_ip_id = 0;
> +uint16_t outer_ip_id = 0;
>  struct dp_packet *seg;
> +const char *data_pos;
>  uint16_t tcp_offset;
>  uint16_t tso_segsz;
>  uint32_t tcp_seq;
> -uint16_t ip_id;
> +bool outer_ipv4;
>  int hdr_len;
>  int seg_len;
> +bool tnl;
>
>  tso_segsz = dp_packet_get_tso_segsz(p);
>  if (!tso_segsz) {
> @@ -105,20 +117,38 @@ dp_packet_gso(struct dp_packet *p, struct 
> dp_packet_batch **batches)
>  return false;
>  }
>
> -tcp_hdr = dp_packet_l4(p);
> -tcp_offset = TCP_OFFSET(tcp_hdr->tcp_ctl);
> -tcp_seq = ntohl(get_16aligned_be32(&tcp_hdr->tcp_seq));
> -hdr_len = ((char *) dp_packet_l4(p) - (char *) dp_packet_eth(p))
> -  + tcp_offset * 4;
> -ip_id = 0;
> -if (dp_packet_hwol_is_ipv4(p)) {
> +if (dp_packet_hwol_is_tunnel_vxlan(p) ||
> +dp_packet_hwol_is_tunnel_geneve(p)) {
> +data_pos =  dp_packet_get_inner_tcp_payload(p);
> +outer_ipv4 = dp_packet_hwol_is_outer_ipv4(p);
> +tcp_hdr = dp_packet_inner_l4(p);
> +ip_hdr = dp_packet_inner_l3(p);
> +tnl = true;
> +
> +if (outer_ipv4) {
> +outer_ip_id = ntohs(((struct ip_header *) 
> dp_packet_l3(p))->ip_id);
> +}
> +if (dp_packet_hwol_is_ipv4(p)) {
> +inner_ip_id = ntohs(ip_hdr->ip_id);
> +}
> +} else {
> +data_pos = dp_packet_get_tcp_payload(p);
> +outer_ipv4 = dp_packet_hwol_is_ipv4(p);
> +tcp_hdr = dp_packet_l4(p);
>  ip_hdr = dp_packet_l3(p);
> -ip_id = ntohs(ip_hdr->ip_id);
> +tnl = false;
> +
> +if (outer_ipv4) {
> +outer_ip_id = ntohs(ip_hdr->ip_id);
> +}
>  }
>
> +tcp_offset = TCP_OFFSET(tcp_hdr->tcp_ctl);
> +tcp_seq = ntohl(get_16aligned_be32(&tcp_hdr->tcp_seq));
> +hdr_len = ((char *) tcp_hdr - (char *) dp_packet_eth(p))
> +  + tcp_offset * 4;
>  const char *data_tail = (char *) dp_packet_tail(p)
>  - dp_packet_l2_pad_size(p);
> -const char *data_pos = dp_packet_get_tcp_payload(p);

Not a strong opinion but data_pos init could be kept here.
const char *data_pos = (char *) tcp_hdr + tcp_offset * 4;


>  int n_segs = dp_packet_gso_nr_segs(p);
>
>  for (int i = 0; i < n_segs; i++) {
> @@ -130,14 +160,35 @@ dp_packet_gso(struct dp_packet *p, struct 
> dp_packet_batch **batches)
>  seg = dp_packet_gso_seg_new(p, hdr_len, data_pos, seg_len);
>  data_pos += seg_len;
>
> +if (tnl) {
> +/* Update tunnel inner L3 header. */
> +if (dp_packet_hwol_is_ipv4(seg)) {
> +ip_hdr = dp_packet_inner_l3(seg);
> +ip_hdr->ip_tot_len = htons(sizeof *ip_hdr +
> +   dp_packet_inner_l4_size(seg));

ip_hdr->ip_tot_len = htons(dp_packet_inner_l3_size(seg)); is easier to
read (and more consistent with the ipv6 block that follows).


> +ip_hdr->ip_id = htons(inner_ip_id);
> +ip_hdr->ip_csum = 0;
> +inner_ip_id++;
> +} else {
> +struct ovs_16aligned_ip6_hdr *ip6_hdr;
> +
> +ip6_hdr = dp_packet_inner_l3(seg);
> +ip6_hdr->ip6_ctlun.ip6_un1.ip6_un1_plen
> += htons(dp_packet_inner_l3_size(seg) - sizeof *ip6_hdr);
> +}
> +}
> +
>  /* Update L3 header. */
> -if (dp_packet_hwol_is_ipv4(seg)) {
> +if (outer_ipv4) {
> +uint16_t ip_tot_len;
>  ip_hdr = dp_packet_l3(seg);
> -ip_hdr->ip_tot_len = htons(sizeof *ip_hdr +
> -   dp_packet_l4_size(seg));
> -ip_hdr->ip_id = htons(ip_id);
> +tnl_hdr = dp_packet_l4(seg);
> +ip_tot_len = sizeof *ip_hdr + dp_packet_l4_size(seg);

ip_hdr->ip_tot_len = htons(dp_packet_l3_size(seg));


> +ip_hdr->ip_tot_len = htons(ip_tot_len);
> +ip_hdr->ip_id = htons(outer_ip_id);
>  ip_hdr->ip_csum = 0;
> -ip_id++;
> +tnl_hdr->udp_len = htons(ip_tot_len - IP_HEADER_LEN);
> +outer_ip_id++;
>  } else {
>  struct ovs_16aligned_ip6_hdr *ip6_hdr = dp_packet_l3(seg);
>


-- 
David Marchand

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

Re: [ovs-dev] [RFC v2 5/9] ofproto-dpif-xlate: Use emit_sample for local sample.

[Ilya Maximets]

On 6/5/24 22:23, Adrian Moreno wrote:
> Use the newly added emit_sample to implement OpenFlow sample() actions
> with local sampling configuration.
> 
> Signed-off-by: Adrian Moreno 
> ---
>  ofproto/ofproto-dpif-lsample.c |  17 
>  ofproto/ofproto-dpif-lsample.h |   6 ++
>  ofproto/ofproto-dpif-xlate.c   | 163 -
>  ofproto/ofproto-dpif-xlate.h   |   3 +-
>  ofproto/ofproto-dpif.c |   2 +-
>  5 files changed, 144 insertions(+), 47 deletions(-)

Not a full review, just a note that this patch needs tests in ofproto-dpif.at
that would check that we generate correct datapath flows.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [RFC v2 9/9] tests: Test local sampling.

[Eelco Chaudron]

On 5 Jun 2024, at 22:23, Adrian Moreno wrote:

> Test simultaneous IPFIX and local sampling including slow-path.
>
> Signed-off-by: Adrian Moreno 
> ---
>  tests/system-common-macros.at |   4 ++
>  tests/system-traffic.at   | 105 ++
>  2 files changed, 109 insertions(+)
>
> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> index 2a68cd664..22b8885e4 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
>  # OVS_CHECK_DROP_ACTION()
>  m4_define([OVS_CHECK_DROP_ACTION],
>  [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
> ovs-vswitchd.log])])
> +
> +# OVS_CHECK_EMIT_SAMPLE()
> +m4_define([OVS_CHECK_EMIT_SAMPLE],
> +[AT_SKIP_IF([! grep -q "Datapath supports emit_sample" 
> ovs-vswitchd.log])])
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index bd7647cbe..babc56b56 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: * 
> * *5002 *2000 *b85e *00
>
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([emit_sample])
> +OVS_TRAFFIC_VSWITCHD_START()
> +OVS_CHECK_EMIT_SAMPLE()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> [ignore])
> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
> [ignore])
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> +
> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66])
> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55])
> +
> +
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg 
> ofproto_dpif_upcall:dbg])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> +-- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \
> +-- create Flow_Sample_Collector_Set id=1 bridge=@br0 
> ipfix=@ipfix, local_sample_group=10 \
> +-- create Flow_Sample_Collector_Set id=2 bridge=@br0 
> ipfix=@ipfix, local_sample_group=12],
> + [0], [ignore])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=ovs-p0,ip 
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100)
> +in_port=ovs-p1,ip 
> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100)
> +])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> +])
> +
> +m4_define([SAMPLE1], [group_id=0xa 
> obs_domain=0x,obs_point=0x 
> .*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])
> +m4_define([SAMPLE2], [group_id=0xc 
> obs_domain=0x,obs_point=0x 
> .*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])
> +AT_CHECK([grep -E 'SAMPLE1' psample.out >/dev/null])
> +AT_CHECK([grep -E 'SAMPLE2' psample.out >/dev/null])
> +
> +AT_CHECK([ovs-ofctl dump-ipfix-flow br0 | sed 's/tx pkts=[[0-9]]*/tx 
> pkts=24/' | sed 's/tx errs=[[0-9]]*/tx errs=0/'], [0], [dnl
> +NXST_IPFIX_FLOW reply (xid=0x2): 2 ids
> +  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
> +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> +  id   2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
> pkts=24
> +  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
> +])

This is not a review, just a heads up so you are aware that this test fails 
sometimes:

+++ 
/home/echaudron/Documents/review/ovs_adrian_psample/tests/system-kmod-testsuite.dir/at-groups/178/stdout
2024-06-24 13:05:35.05700 +0200
@@ -1,6 +1,6 @@
 NXST_IPFIX_FLOW reply (xid=0x2): 2 ids
-  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
pkts=24
-  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
   id   2: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
pkts=24
   pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0
+  id   1: flows=1, current flows=0, sampled pkts=1, ipv4 ok=1, ipv6 ok=0, tx 
pkts=24
+  pkts errs=0, ipv4 errs=0, ipv6 errs=0, tx errs=0

The order does not seem to be fixed all the time (same for the one below).

//Eelco

> +
> +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl
> +local sample statistics for bridge "br0":
> +- Collector Set ID: 1
> +  Local Sample Group: 10
> +  Total number of bytes: 98
> +  Total number of packets: 1
> +
> +- Collector Set ID: 2
> +  Local Sample Group: 12
> +  Total number of bytes: 98
> +  Total number of packets: 1
> +])
> +
> +# Disable trunc feature to force traffic to go through slo

Re: [ovs-dev] [PATCH ovn 2/2] ci: Move DPDK build into container.

[Ales Musil]

On Mon, Jun 24, 2024 at 11:33 AM Ilya Maximets  wrote:

> On 6/20/24 14:57, Ales Musil wrote:
> > The DPDK was built as extra step in the CI, however it is useful to
> > have it inside the container prepared. This also helps with
> > reproduction of failures with DPDK by having the exact version inside
> > the container already.
> >
> > Also bump the Ubuntu version for container builds to 24.04 to get
> > Podman 4. This is required to avoid the tar permissions error:
> >
> > tar: dpdk-23.11/.ci: Cannot change mode to rwxrwxr-x: Operation not
> permitted
> >
> > Signed-off-by: Ales Musil 
> > ---
> >  .ci/ci.sh|  6 --
> >  .ci/dpdk-build.sh| 62 --
> >  .ci/dpdk-prepare.sh  | 11 
> >  .ci/linux-build.sh   |  7 +-
> >  .github/workflows/containers.yml |  2 +-
> >  .github/workflows/test.yml   | 81 +---
> >  Makefile.am  |  2 -
> >  utilities/containers/fedora/Dockerfile   |  1 +
> >  utilities/containers/prepare.sh  | 56 
> >  utilities/containers/py-requirements.txt |  1 +
> >  utilities/containers/ubuntu/Dockerfile   |  1 +
> >  11 files changed, 66 insertions(+), 164 deletions(-)
> >  delete mode 100755 .ci/dpdk-build.sh
> >  delete mode 100755 .ci/dpdk-prepare.sh
> >
> > diff --git a/.ci/ci.sh b/.ci/ci.sh
> > index 6beeace84..f543967dc 100755
> > --- a/.ci/ci.sh
> > +++ b/.ci/ci.sh
> > @@ -16,7 +16,6 @@
> >
> >  OVN_PATH=${OVN_PATH:-$PWD}
> >  OVS_PATH=${OVS_PATH:-$OVN_PATH/ovs}
> > -DPDK_PATH=${DPDK_PATH:-$OVN_PATH/dpdk-dir}
> >  CONTAINER_CMD=${CONTAINER_CMD:-podman}
> >  CONTAINER_WORKSPACE="/workspace"
> >  CONTAINER_WORKDIR="/workspace/ovn-tmp"
> > @@ -163,17 +162,12 @@ if [ "$ARCH" = "aarch64" ] && !
> check_clang_version_ge "16.0.0"; then
> >  ASAN_OPTIONS="detect_leaks=0"
> >  fi
> >
> > -if [ -z "$DPDK" ]; then
> > -   mkdir -p "$DPDK_PATH"
> > -fi
> > -
> >  CONTAINER_ID="$($CONTAINER_CMD run --privileged -d \
> >  --pids-limit=-1 \
> >  --env ASAN_OPTIONS=$ASAN_OPTIONS \
> >  -v /lib/modules/$(uname -r):/lib/modules/$(uname -r):ro \
> >  -v $OVN_PATH:$CONTAINER_WORKSPACE/ovn:Z \
> >  -v $OVS_PATH:$CONTAINER_WORKSPACE/ovs:Z \
> > --v $DPDK_PATH:$CONTAINER_WORKSPACE/dpdk-dir:Z \
> >  $IMAGE_NAME)"
> >  trap remove_container EXIT
> >
> > diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
> > deleted file mode 100755
> > index 0c13c98c9..0
> > --- a/.ci/dpdk-build.sh
> > +++ /dev/null
> > @@ -1,62 +0,0 @@
> > -#!/bin/bash
> > -
> > -set -o errexit
> > -set -x
> > -
> > -function build_dpdk()
> > -{
> > -local DPDK_VER=$1
> > -local DPDK_OPTS=""
> > -local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> > -local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
> > -
> > -rm -rf dpdk-src
> > -rm -rf $DPDK_INSTALL_DIR
> > -
> > -if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
> > -git clone --single-branch $DPDK_GIT dpdk-src -b
> "${DPDK_VER##refs/*/}"
> > -pushd dpdk-src
> > -git log -1 --oneline
> > -else
> > -wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
> > -tar xvf dpdk-$1.tar.xz > /dev/null
> > -DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
> > -mv ${DIR_NAME} dpdk-src
> > -pushd dpdk-src
> > -fi
> > -
> > -# Switching to 'default' machine to make the dpdk cache usable on
> > -# different CPUs. We can't be sure that all CI machines are exactly
> same.
> > -DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
> > -
> > -# Disable building DPDK unit tests. Not needed for OVS build or
> tests.
> > -DPDK_OPTS="$DPDK_OPTS -Dtests=false"
> > -
> > -# Disable DPDK developer mode, this results in less build checks
> and less
> > -# meson verbose outputs.
> > -DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
> > -
> > -# OVS compilation and the "ovn-system-dpdk" unit tests (run in the
> CI)
> > -# only depend on virtio/tap drivers.
> > -# We can disable all remaining drivers to save compilation time.
> > -DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
> > -# OVS depends on the vhost library (and its dependencies).
> > -# net/tap depends on the gso library.
> > -DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
> > -
> > -# Install DPDK using prefix.
> > -DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
> > -
> > -meson $DPDK_OPTS build
> > -ninja -C build
> > -ninja -C build install
> > -popd
> > -
> > -# Remove examples sources.
> > -rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
> > -
> > -echo "Installed DPDK in $DPDK_INSTALL_DIR"
> > -echo "${DPDK_VER}" > ${VERSION_FILE}
> > -}
> > -
> > -build_dpdk $DPDK_VER
> > diff --git a/.ci/dpdk-prepare.sh b/.ci/dpdk-prepare.sh
> > deleted file mode 100755
> > index 5543da90a..0
> > -

Re: [ovs-dev] [PATCH ovn] ci: Use user for pip upgrade on Ubuntu.

[Ales Musil]

On Mon, Jun 24, 2024 at 11:52 AM Ilya Maximets  wrote:

> On 6/24/24 11:24, Dumitru Ceara wrote:
> > On 6/24/24 11:00, Ales Musil wrote:
> >> The pip upgrade in Ubuntu started to fail because of missing
> >> RECORD file. Use the --user argument which avoids this error and
> >> allows pip to upgrade itself.
> >>
> >> ERROR: Cannot uninstall pip 24.0, RECORD file not found.
> >> Hint: The package was installed by debian.
> >>
> >> Suggested-by: Ilya Maximets 
> >> Signed-off-by: Ales Musil 
> >> ---
> >
> > Thanks, Ales, for fixing the CI!
> >
> >>  utilities/containers/ubuntu/Dockerfile | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/utilities/containers/ubuntu/Dockerfile
> b/utilities/containers/ubuntu/Dockerfile
> >> index ce7ce16c6..0e71b1a02 100755
> >> --- a/utilities/containers/ubuntu/Dockerfile
> >> +++ b/utilities/containers/ubuntu/Dockerfile
> >> @@ -80,7 +80,7 @@ COPY $CONTAINERS_PATH/py-requirements.txt
> /tmp/py-requirements.txt
> >>  ENV PIP_BREAK_SYSTEM_PACKAGES 1
> >>
> >>  # Update and install pip dependencies
> >> -RUN python3 -m pip install --upgrade pip \
> >> +RUN python3 -m pip install --upgrade --user pip \
> >>  && \
> >>  python3 -m pip install wheel \
> >>  && \
> >
> > I think for consistency we should do the same thing for the Fedora
> > dockerfile.  However, you're changing all that in:
> >
> > https://patchwork.ozlabs.org/project/ovn/list/?series=411795&state=*
> >
> > I think it's probably better to do all that at once (to avoid having to
> > rebase different series) and submit a v2 of the series that moves the
> > DPDK build into the container.  That series could have as first patch
> > the --user change for both dockerfiles.
> >
> > What do you think?
>
> This also should be done for all 'pip install' calls, not only the
> upgrade of a pip itself.
>
> Best regards, Ilya Maximets.
>
>
Sure sounds good, I'll prepare v2 of the DPDK series with this fix included.

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

Re: [ovs-dev] [PATCH ovn] ci: Use user for pip upgrade on Ubuntu.

[Ilya Maximets]

On 6/24/24 11:24, Dumitru Ceara wrote:
> On 6/24/24 11:00, Ales Musil wrote:
>> The pip upgrade in Ubuntu started to fail because of missing
>> RECORD file. Use the --user argument which avoids this error and
>> allows pip to upgrade itself.
>>
>> ERROR: Cannot uninstall pip 24.0, RECORD file not found.
>> Hint: The package was installed by debian.
>>
>> Suggested-by: Ilya Maximets 
>> Signed-off-by: Ales Musil 
>> ---
> 
> Thanks, Ales, for fixing the CI!
> 
>>  utilities/containers/ubuntu/Dockerfile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/utilities/containers/ubuntu/Dockerfile 
>> b/utilities/containers/ubuntu/Dockerfile
>> index ce7ce16c6..0e71b1a02 100755
>> --- a/utilities/containers/ubuntu/Dockerfile
>> +++ b/utilities/containers/ubuntu/Dockerfile
>> @@ -80,7 +80,7 @@ COPY $CONTAINERS_PATH/py-requirements.txt 
>> /tmp/py-requirements.txt
>>  ENV PIP_BREAK_SYSTEM_PACKAGES 1
>>  
>>  # Update and install pip dependencies
>> -RUN python3 -m pip install --upgrade pip \
>> +RUN python3 -m pip install --upgrade --user pip \
>>  && \
>>  python3 -m pip install wheel \
>>  && \
> 
> I think for consistency we should do the same thing for the Fedora
> dockerfile.  However, you're changing all that in:
> 
> https://patchwork.ozlabs.org/project/ovn/list/?series=411795&state=*
> 
> I think it's probably better to do all that at once (to avoid having to
> rebase different series) and submit a v2 of the series that moves the
> DPDK build into the container.  That series could have as first patch
> the --user change for both dockerfiles.
> 
> What do you think?

This also should be done for all 'pip install' calls, not only the
upgrade of a pip itself.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH ovn 2/2] ci: Move DPDK build into container.

[Ilya Maximets]

On 6/20/24 14:57, Ales Musil wrote:
> The DPDK was built as extra step in the CI, however it is useful to
> have it inside the container prepared. This also helps with
> reproduction of failures with DPDK by having the exact version inside
> the container already.
> 
> Also bump the Ubuntu version for container builds to 24.04 to get
> Podman 4. This is required to avoid the tar permissions error:
> 
> tar: dpdk-23.11/.ci: Cannot change mode to rwxrwxr-x: Operation not permitted
> 
> Signed-off-by: Ales Musil 
> ---
>  .ci/ci.sh|  6 --
>  .ci/dpdk-build.sh| 62 --
>  .ci/dpdk-prepare.sh  | 11 
>  .ci/linux-build.sh   |  7 +-
>  .github/workflows/containers.yml |  2 +-
>  .github/workflows/test.yml   | 81 +---
>  Makefile.am  |  2 -
>  utilities/containers/fedora/Dockerfile   |  1 +
>  utilities/containers/prepare.sh  | 56 
>  utilities/containers/py-requirements.txt |  1 +
>  utilities/containers/ubuntu/Dockerfile   |  1 +
>  11 files changed, 66 insertions(+), 164 deletions(-)
>  delete mode 100755 .ci/dpdk-build.sh
>  delete mode 100755 .ci/dpdk-prepare.sh
> 
> diff --git a/.ci/ci.sh b/.ci/ci.sh
> index 6beeace84..f543967dc 100755
> --- a/.ci/ci.sh
> +++ b/.ci/ci.sh
> @@ -16,7 +16,6 @@
>  
>  OVN_PATH=${OVN_PATH:-$PWD}
>  OVS_PATH=${OVS_PATH:-$OVN_PATH/ovs}
> -DPDK_PATH=${DPDK_PATH:-$OVN_PATH/dpdk-dir}
>  CONTAINER_CMD=${CONTAINER_CMD:-podman}
>  CONTAINER_WORKSPACE="/workspace"
>  CONTAINER_WORKDIR="/workspace/ovn-tmp"
> @@ -163,17 +162,12 @@ if [ "$ARCH" = "aarch64" ] && ! check_clang_version_ge 
> "16.0.0"; then
>  ASAN_OPTIONS="detect_leaks=0"
>  fi
>  
> -if [ -z "$DPDK" ]; then
> -   mkdir -p "$DPDK_PATH"
> -fi
> -
>  CONTAINER_ID="$($CONTAINER_CMD run --privileged -d \
>  --pids-limit=-1 \
>  --env ASAN_OPTIONS=$ASAN_OPTIONS \
>  -v /lib/modules/$(uname -r):/lib/modules/$(uname -r):ro \
>  -v $OVN_PATH:$CONTAINER_WORKSPACE/ovn:Z \
>  -v $OVS_PATH:$CONTAINER_WORKSPACE/ovs:Z \
> --v $DPDK_PATH:$CONTAINER_WORKSPACE/dpdk-dir:Z \
>  $IMAGE_NAME)"
>  trap remove_container EXIT
>  
> diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
> deleted file mode 100755
> index 0c13c98c9..0
> --- a/.ci/dpdk-build.sh
> +++ /dev/null
> @@ -1,62 +0,0 @@
> -#!/bin/bash
> -
> -set -o errexit
> -set -x
> -
> -function build_dpdk()
> -{
> -local DPDK_VER=$1
> -local DPDK_OPTS=""
> -local DPDK_INSTALL_DIR="$(pwd)/dpdk-dir"
> -local VERSION_FILE="$DPDK_INSTALL_DIR/cached-version"
> -
> -rm -rf dpdk-src
> -rm -rf $DPDK_INSTALL_DIR
> -
> -if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
> -git clone --single-branch $DPDK_GIT dpdk-src -b 
> "${DPDK_VER##refs/*/}"
> -pushd dpdk-src
> -git log -1 --oneline
> -else
> -wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
> -tar xvf dpdk-$1.tar.xz > /dev/null
> -DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
> -mv ${DIR_NAME} dpdk-src
> -pushd dpdk-src
> -fi
> -
> -# Switching to 'default' machine to make the dpdk cache usable on
> -# different CPUs. We can't be sure that all CI machines are exactly same.
> -DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
> -
> -# Disable building DPDK unit tests. Not needed for OVS build or tests.
> -DPDK_OPTS="$DPDK_OPTS -Dtests=false"
> -
> -# Disable DPDK developer mode, this results in less build checks and less
> -# meson verbose outputs.
> -DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
> -
> -# OVS compilation and the "ovn-system-dpdk" unit tests (run in the CI)
> -# only depend on virtio/tap drivers.
> -# We can disable all remaining drivers to save compilation time.
> -DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
> -# OVS depends on the vhost library (and its dependencies).
> -# net/tap depends on the gso library.
> -DPDK_OPTS="$DPDK_OPTS -Denable_libs=cryptodev,dmadev,gso,vhost"
> -
> -# Install DPDK using prefix.
> -DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
> -
> -meson $DPDK_OPTS build
> -ninja -C build
> -ninja -C build install
> -popd
> -
> -# Remove examples sources.
> -rm -rf $DPDK_INSTALL_DIR/share/dpdk/examples
> -
> -echo "Installed DPDK in $DPDK_INSTALL_DIR"
> -echo "${DPDK_VER}" > ${VERSION_FILE}
> -}
> -
> -build_dpdk $DPDK_VER
> diff --git a/.ci/dpdk-prepare.sh b/.ci/dpdk-prepare.sh
> deleted file mode 100755
> index 5543da90a..0
> --- a/.ci/dpdk-prepare.sh
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -#!/bin/bash
> -
> -set -ev
> -
> -# Installing wheel separately because it may be needed to build some
> -# of the packages during dependency backtracking and pip >= 22.0 will
> -# stop backtracking on build failures:
> -# https://github.com/pypa/

Re: [ovs-dev] [PATCH v4] netdev-dpdk: Use LSC interrupt mode.

[Ilya Maximets]

On 6/19/24 19:29, Kevin Traynor wrote:
> On 19/06/2024 17:00, David Marchand wrote:
>> Querying link status may get delayed for an undeterministic (long) time
>> with mlx5 ports. This is a consequence of the mlx5 driver calling ethtool
>> kernel API and getting stuck on the kernel RTNL lock while some other
>> operation is in progress under this lock.
>>
>> One impact for long link status query is that it is called under the bond
>> lock taken in write mode periodically in bond_run().
>> In parallel, datapath threads may block requesting to read bonding related
>> info (like for example in bond_check_admissibility()).
>>
>> The LSC interrupt mode is available with many DPDK drivers and is used by
>> default with testpmd.
>>
>> It seems safe enough to switch on this feature by default in OVS.
>> We keep the per interface option to disable this feature in case of an
>> unforeseen bug.
>>
>> Signed-off-by: David Marchand 
>> Reviewed-by: Robin Jarry 
>> Acked-by: Mike Pattrick 
>> ---
> 
> LGTM
> 
> Acked-by: Kevin Traynor 
> 
> Re backporting. I'm not so keen on changing a default when someone
> upgrades from 3.3.1 -> 3.3.2 etc. The feature is already available so
> anyone who needs it can enable it.

+1

Changing defaults in stable branches is not a good practice.  Since
the behavior was there for a very long time and there is a workaround
for those who hit the issue, it should be enough.

I'd suggest considering the change inside the DPDK itself, i.e. enabling
LSC by default for all or problematic (mlx) drivers.

For the patch itself, I'm not sure why WARN and DBG messages are worded
differently.  I'd prefer them to be the same (as in WARN).  But that's
a minor nit that can be fixed on commit, if you think it is needed.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ci: Use user for pip upgrade on Ubuntu.

[Dumitru Ceara]

On 6/24/24 11:00, Ales Musil wrote:
> The pip upgrade in Ubuntu started to fail because of missing
> RECORD file. Use the --user argument which avoids this error and
> allows pip to upgrade itself.
> 
> ERROR: Cannot uninstall pip 24.0, RECORD file not found.
> Hint: The package was installed by debian.
> 
> Suggested-by: Ilya Maximets 
> Signed-off-by: Ales Musil 
> ---

Thanks, Ales, for fixing the CI!

>  utilities/containers/ubuntu/Dockerfile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utilities/containers/ubuntu/Dockerfile 
> b/utilities/containers/ubuntu/Dockerfile
> index ce7ce16c6..0e71b1a02 100755
> --- a/utilities/containers/ubuntu/Dockerfile
> +++ b/utilities/containers/ubuntu/Dockerfile
> @@ -80,7 +80,7 @@ COPY $CONTAINERS_PATH/py-requirements.txt 
> /tmp/py-requirements.txt
>  ENV PIP_BREAK_SYSTEM_PACKAGES 1
>  
>  # Update and install pip dependencies
> -RUN python3 -m pip install --upgrade pip \
> +RUN python3 -m pip install --upgrade --user pip \
>  && \
>  python3 -m pip install wheel \
>  && \

I think for consistency we should do the same thing for the Fedora
dockerfile.  However, you're changing all that in:

https://patchwork.ozlabs.org/project/ovn/list/?series=411795&state=*

I think it's probably better to do all that at once (to avoid having to
rebase different series) and submit a v2 of the series that moves the
DPDK build into the container.  That series could have as first patch
the --user change for both dockerfiles.

What do you think?

Regards,
Dumitru

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

[ovs-dev] [PATCH ovn] ci: Use user for pip upgrade on Ubuntu.

[Ales Musil]

The pip upgrade in Ubuntu started to fail because of missing
RECORD file. Use the --user argument which avoids this error and
allows pip to upgrade itself.

ERROR: Cannot uninstall pip 24.0, RECORD file not found.
Hint: The package was installed by debian.

Suggested-by: Ilya Maximets 
Signed-off-by: Ales Musil 
---
 utilities/containers/ubuntu/Dockerfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utilities/containers/ubuntu/Dockerfile 
b/utilities/containers/ubuntu/Dockerfile
index ce7ce16c6..0e71b1a02 100755
--- a/utilities/containers/ubuntu/Dockerfile
+++ b/utilities/containers/ubuntu/Dockerfile
@@ -80,7 +80,7 @@ COPY $CONTAINERS_PATH/py-requirements.txt 
/tmp/py-requirements.txt
 ENV PIP_BREAK_SYSTEM_PACKAGES 1
 
 # Update and install pip dependencies
-RUN python3 -m pip install --upgrade pip \
+RUN python3 -m pip install --upgrade --user pip \
 && \
 python3 -m pip install wheel \
 && \
-- 
2.45.1

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

Re: [ovs-dev] [PATCH v3 ovn 3/3] ofctrl: Introduce ecmp_nexthop_monitor.

[Ales Musil]

On Thu, Jun 6, 2024 at 8:35 PM Lorenzo Bianconi 
wrote:

> Introduce ecmp_nexthop_monitor in ovn-controller in order to track and
> flush ecmp-symmetric reply ct entires when requested by the CMS (e.g
> removing the related static routes).
>
> Signed-off-by: Lorenzo Bianconi 
> ---
>

Hi Lorenzo,

thank you for the patch. I have some comments below.


>  controller/ofctrl.c | 101 ++
>  controller/ofctrl.h |   2 +
>  controller/ovn-controller.c |   2 +
>  tests/system-ovn-kmod.at| 266 
>  tests/system-ovn.at |   4 +
>  5 files changed, 375 insertions(+)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 9d181a782..826f78a85 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -388,9 +388,24 @@ struct meter_band_entry {
>
>  static struct shash meter_bands;
>
> +static struct hmap ecmp_nexthop_map;
> +struct ecmp_nexthop_entry {
> +struct hmap_node node;
> +bool erase;
> +
> +char *nexthop;
> +int id;
> +};
> +
>

Do we actually need this struct? It should be enough to keep the up-to-date
ids inside of bitmap.


>  static void ofctrl_meter_bands_destroy(void);
>  static void ofctrl_meter_bands_clear(void);
>
> +static void ecmp_nexthop_monitor_destroy(void);
> +static void ecmp_nexthop_monitor_run(
> +const struct sbrec_ecmp_nexthop_table *enh_table,
> +struct ovs_list *msgs);
> +
> +
>  /* MFF_* field ID for our Geneve option.  In S_TLV_TABLE_MOD_SENT, this is
>   * the option we requested (we don't know whether we obtained it yet).  In
>   * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
> @@ -429,6 +444,7 @@ ofctrl_init(struct ovn_extend_table *group_table,
>  groups = group_table;
>  meters = meter_table;
>  shash_init(&meter_bands);
> +hmap_init(&ecmp_nexthop_map);
>  }
>
>  /* S_NEW, for a new connection.
> @@ -876,6 +892,7 @@ ofctrl_destroy(void)
>  expr_symtab_destroy(&symtab);
>  shash_destroy(&symtab);
>  ofctrl_meter_bands_destroy();
> +ecmp_nexthop_monitor_destroy();
>  }
>
>  uint64_t
> @@ -2305,6 +2322,87 @@ add_meter(struct ovn_extend_table_info *m_desired,
>  ofctrl_meter_bands_alloc(sb_meter, m_desired, msgs);
>  }
>
> +static void
> +ecmp_nexthop_monitor_free_entry(struct ecmp_nexthop_entry *e,
> +struct ovs_list *msgs)
> +{
> +if (msgs) {
> +ovs_u128 mask = {
> +/* ct_labels.label BITS[96-127] */
> +.u64.hi = 0x,
> +};
> +uint64_t id = e->id;
> +ovs_u128 nexthop = {
> +.u64.hi = id << 32,
> +};
> +struct ofp_ct_match match = {
> +.labels = nexthop,
> +.labels_mask = mask,
> +};
> +struct ofpbuf *msg = ofp_ct_match_encode(&match, NULL,
> +
>  rconn_get_version(swconn));
> +ovs_list_push_back(msgs, &msg->list_node);
> +}
> +free(e->nexthop);
> +free(e);
> +}
> +
> +static void
> +ecmp_nexthop_monitor_destroy(void)
> +{
> +struct ecmp_nexthop_entry *e;
> +HMAP_FOR_EACH_POP (e, node, &ecmp_nexthop_map) {
> +ecmp_nexthop_monitor_free_entry(e, NULL);
> +}
> +hmap_destroy(&ecmp_nexthop_map);
> +}
> +
> +static struct ecmp_nexthop_entry *
> +ecmp_nexthop_monitor_lookup(char *nexthop)
> +{
> +uint32_t hash = hash_string(nexthop, 0);
> +struct ecmp_nexthop_entry *e;
> +
> +HMAP_FOR_EACH_WITH_HASH (e, node, hash, &ecmp_nexthop_map) {
> +if (!strcmp(e->nexthop, nexthop)) {
> +return e;
> +}
> +}
> +return NULL;
> +}
> +
> +static void
> +ecmp_nexthop_monitor_run(const struct sbrec_ecmp_nexthop_table *enh_table,
> + struct ovs_list *msgs)
> +{
> +struct ecmp_nexthop_entry *e;
> +HMAP_FOR_EACH (e, node, &ecmp_nexthop_map) {
> +e->erase = true;
> +}
> +
> +const struct sbrec_ecmp_nexthop *sbrec_ecmp_nexthop;
> +SBREC_ECMP_NEXTHOP_TABLE_FOR_EACH (sbrec_ecmp_nexthop, enh_table) {
> +e = ecmp_nexthop_monitor_lookup(sbrec_ecmp_nexthop->nexthop);
> +if (!e) {
> +e = xzalloc(sizeof *e);
> +e->nexthop = xstrdup(sbrec_ecmp_nexthop->nexthop);
> +e->id = sbrec_ecmp_nexthop->id;
> +uint32_t hash = hash_string(e->nexthop, 0);
> +hmap_insert(&ecmp_nexthop_map, &e->node, hash);
> +} else {
> +e->erase = false;
> +}
> +}
> +
> +HMAP_FOR_EACH_SAFE (e, node, &ecmp_nexthop_map) {
> +if (e->erase) {
> +hmap_remove(&ecmp_nexthop_map, &e->node);
> +ecmp_nexthop_monitor_free_entry(e, msgs);
> +}
> +}
> +
> +}
> +
>  static void
>  installed_flow_add(struct ovn_flow *d,
> struct ofputil_bundle_ctrl_msg *bc,
> @@ -2663,6 +2761,7 @@ ofctrl_put(struct ovn_desired_flow_table
> *lflow_table,
> struct shash *pending_ct_zones,
>

Re: [ovs-dev] [PATCH v3 ovn 2/3] northd: Add nexhop id in ct_label.label.

[Ales Musil]

On Thu, Jun 6, 2024 at 8:35 PM 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 
>

Hi Lorenzo,

thank you for the patch. I have one small nit down below.


> ---
>  northd/en-lflow.c|  3 +++
>  northd/inc-proc-northd.c |  1 +
>  northd/northd.c  | 41 +---
>  northd/northd.h  |  1 +
>  tests/ovn.at |  4 +--
>  tests/system-ovn.at  | 58 +++-
>  6 files changed, 72 insertions(+), 36 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 3dba5034b..b4df49076 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -54,6 +54,8 @@ lflow_get_input_data(struct engine_node *node,
>  engine_get_input_data("lr_stateful", node);
>  struct ed_type_ls_stateful *ls_stateful_data =
>  engine_get_input_data("ls_stateful", node);
> +struct ecmp_nexthop_data *nexthop_data =
> +engine_get_input_data("ecmp_nexthop", node);
>
>  lflow_input->sbrec_logical_flow_table =
>  EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
> @@ -83,6 +85,7 @@ lflow_get_input_data(struct engine_node *node,
>  lflow_input->parsed_routes = &static_routes_data->parsed_routes;
>  lflow_input->route_tables = &static_routes_data->route_tables;
>  lflow_input->route_policies = &route_policies_data->route_policies;
> +lflow_input->nexthops_table = &nexthop_data->nexthops;
>
>  struct ed_type_global_config *global_config =
>  engine_get_input_data("global_config", node);
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index c4e5b9bf6..3d4bfa175 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -281,6 +281,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>  engine_add_input(&en_lflow, &en_route_policies, NULL);
>  engine_add_input(&en_lflow, &en_static_routes, NULL);
>  engine_add_input(&en_lflow, &en_bfd, NULL);
> +engine_add_input(&en_lflow, &en_ecmp_nexthop, NULL);
>  engine_add_input(&en_lflow, &en_northd, lflow_northd_handler);
>  engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler);
>  engine_add_input(&en_lflow, &en_lr_stateful,
> lflow_lr_stateful_handler);
> diff --git a/northd/northd.c b/northd/northd.c
> index efe1e3f46..0e7ff0df1 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10903,7 +10903,8 @@ add_ecmp_symmetric_reply_flows(struct lflow_table
> *lflows,
> struct ovn_port *out_port,
> const struct parsed_route *route,
> struct ds *route_match,
> -   struct lflow_ref *lflow_ref)
> +   struct lflow_ref *lflow_ref,
> +   struct hmap *nexthops_table)
>  {
>  const struct nbrec_logical_router_static_route *st_route =
> route->route;
>  struct ds match = DS_EMPTY_INITIALIZER;
> @@ -10939,15 +10940,28 @@ add_ecmp_symmetric_reply_flows(struct
> lflow_table *lflows,
>   * ds_put_cstr() call. The previous contents are needed.
>   */
>  ds_put_cstr(&match, " && !ct.rpl && (ct.new || ct.est)");
> +struct ds nexthop_label = DS_EMPTY_INITIALIZER;
>

nit: There isn't any need for the extra "struct ds". You can find the entry
in the loop below and use it in ds_put_format directly.

+
> +struct ecmp_nexthop_entry *e;
> +HMAP_FOR_EACH_WITH_HASH (e, hmap_node, hash_string(st_route->nexthop,
> 0),
> + nexthops_table) {
> +if (!strcmp(st_route->nexthop, e->nexthop)) {
> +ds_put_format(&nexthop_label, "ct_label.label = %d;", e->id);
> +break;
> +}
> +}
> +
>  ds_put_format(&actions,
>  "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(&nexthop_label));
>  ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 100,
>  ds_cstr(&match), ds_cstr(&actions),
>  &st_route->header_,
>  lflow_ref);
> +ds_destroy(&nexthop_label);
>
>  /* Bypass ECMP selection if we already have ct_label information
>   * for where to route the packet.
> @@ -11001,7 +11015,8 @@ static void
>  build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
>bool ct_masked_mark, const struct hmap *lr_ports,
>

Re: [ovs-dev] [PATCH v3 ovn 1/3] northd: Introduce ECMP_Nexthop table in SB db.

[Ales Musil]

On Thu, Jun 6, 2024 at 8:34 PM 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 
> ---
>

Hi Lorenzo,

thank you for the patch. I have a couple of comments see below.


>  northd/en-northd.c   |  33 +++
>  northd/en-northd.h   |   4 ++
>  northd/inc-proc-northd.c |   7 ++-
>  northd/northd.c  | 117 +++
>  northd/northd.h  |  10 
>  ovn-sb.ovsschema |  16 +-
>  ovn-sb.xml   |  31 +++
>  tests/ovn-northd.at  |   4 ++
>  8 files changed, 220 insertions(+), 2 deletions(-)
>
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index a4de71ba1..a2823ab2b 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -380,6 +380,23 @@ en_bfd_run(struct engine_node *node, void *data)
>  engine_set_node_state(node, EN_UPDATED);
>  }
>
> +void
> +en_ecmp_nexthop_run(struct engine_node *node, void *data)
> +{
> +const struct engine_context *eng_ctx = engine_get_context();
> +struct static_routes_data *static_routes_data =
> +engine_get_input_data("static_routes", node);
> +struct ecmp_nexthop_data *enh_data = data;
> +const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table =
> +EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node));
> +
> +build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn,
> + &static_routes_data->parsed_routes,
> + &enh_data->nexthops,
> + sbrec_ecmp_nexthop_table);
> +engine_set_node_state(node, EN_UPDATED);
> +}
> +
>  void
>  *en_northd_init(struct engine_node *node OVS_UNUSED,
>  struct engine_arg *arg OVS_UNUSED)
> @@ -421,6 +438,16 @@ void
>  return data;
>  }
>
> +void
> +*en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
> +  struct engine_arg *arg OVS_UNUSED)
> +{
> +struct ecmp_nexthop_data *data = xzalloc(sizeof *data);
> +
> +ecmp_nexthop_init(data);
> +return data;
> +}
> +
>  void
>  en_northd_cleanup(void *data)
>  {
> @@ -451,3 +478,9 @@ en_bfd_cleanup(void *data)
>  {
>  bfd_destroy(data);
>  }
> +
> +void
> +en_ecmp_nexthop_cleanup(void *data)
> +{
> +ecmp_nexthop_destroy(data);
> +}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 424209c2f..c6d520f71 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -34,5 +34,9 @@ void *en_bfd_init(struct engine_node *node OVS_UNUSED,
>  void en_bfd_cleanup(void *data);
>  bool bfd_change_handler(struct engine_node *node, void *data);
>  void en_bfd_run(struct engine_node *node, void *data);
> +void en_ecmp_nexthop_run(struct engine_node *node, void *data);
> +void *en_ecmp_nexthop_init(struct engine_node *node OVS_UNUSED,
> +   struct engine_arg *arg OVS_UNUSED);
> +void en_ecmp_nexthop_cleanup(void *data);
>
>  #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index d907da14d..c4e5b9bf6 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -103,7 +103,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,
> @@ -160,6 +161,7 @@ static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ls_stateful,
> "ls_stateful");
>  static ENGINE_NODE(route_policies, "route_policies");
>  static ENGINE_NODE(static_routes, "static_routes");
>  static ENGINE_NODE(bfd, "bfd");
> +static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop");
>
>  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>struct ovsdb_idl_loop *sb)
> @@ -261,6 +263,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>  engine_add_input(&en_static_routes,
>   &en_nb_logical_router_static_route, NULL);
>
> +engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL);
> +engine_add_input(&en_ecmp_nexthop, &en_static_routes, NULL);
> +
>  engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
>  engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
>  engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 2eb5f2be8..efe1e3f46 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10039,6 +10039,105 @@ build_bfd_table(
>  return ret;
>  }
>
> +struct ecmp_nexthop_entry {
> +struct hmap_node hmap_node;
> +
> +char *nexthop;
> +int id;
> +bool stale;
> +};
>

I feel like simap would be enough in this case, basically anything that