Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On 22 Jan 2024, at 21:51, Ilya Maximets wrote:
> On 1/19/24 14:01, Eelco Chaudron wrote:
>>
>>
>> On 19 Jan 2024, at 13:53, David Marchand wrote:
>>
>>> On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets wrote:
On 1/18/24 14:00, David Marchand wrote:
> Seen in GHA recently.
> Unit tests are checking conntracks relating to a destination ip address
> but the FORMAT_CT macro is not strict enough and would match unrelated
> conntracks too.
>
> Example:
> 148. system-traffic.at:6432: testing conntrack - DNAT with
> additional SNAT ...
> [...]
> ./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
> grep "dst=10.1.1.1" |
> sed -e 's/port=[0-9]*/port=/g'
> -e 's/id=[0-9]*/id=/g'
> -e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
> [...]
> @@ -1,2 +1,7 @@
> tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
> +tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
> +tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
> +tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
>
> Fixes: 07659514c3c1 ("Add support for connection tracking.")
> Signed-off-by: David Marchand
> ---
> tests/system-common-macros.at | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
> index 01ebe364ee..07be29f673 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
> 's/csum:.*/csum: /'])
> # and limit the output to the rows containing 'ip-addr'.
> #
> m4_define([FORMAT_CT],
> -[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> sort | uniq]])
> +[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
> 's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
> sort | uniq]])
>
> # NETNS_DAEMONIZE([namespace], [command], [pidfile])
> #
I remembered why the macro is loose. We wanted to be able
to match on "subnets" by supplying only part of the address.
There was at least one test that used this functionality.
Eelco removed it though here:
https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9
Did you check if have any more instances of such tests?
>>>
>>> I did not.
>>>
They can be tricky to find, as we can supply 10.1.1.2 in order
to match 10.1.1.240, for example.
>>>
>>> Ok, you can discard my patch.
>>> Thanks.
>>
>> But looking at most of the test cases when they put in an IP they
>> mean that specific IP not 10.1.1.20?
>
> The key word here is 'most', it's hard to tell for all of them without
> actually analyzing the logic of each of them.
>
>> But maybe your NS idea works better.
>
> It should fix the issue at hands. So I marked this one as rejected for
> now. We can revisit it later if the need arises.
>
> If we move to separate namespaces per test it may be possible to just
> remove filtering entirely and check the exact content of conntrack
> tables instead, I hope.
ACK, this would be nice. It might also allow us to run the dp tests in parallel.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] dp-packet: Reset offload flags when clearing a packet.
The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
a BFD packet flagged as double encapsulated would trigger a seg fault.
The problem surfaced because bfd_put_packet was reusing a packet
allocated on the stack that wasn't having its flags reset between calls.
This change will reset OL flags in data_clear(), which should fix this
type of packet reuse issue in general as long as data_clear() is called
in between uses. This change also includes a tangentially related check
in dp_packet_inner_l4_size(), where the correct offset was not being
checked.
Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
Fixes: 85bcbbed839a ("userspace: Enable tunnel tests with TSO.")
Reported-by: Dumitru Ceara
Reported-at: https://issues.redhat.com/browse/FDP-300
Signed-off-by: Mike Pattrick
---
lib/dp-packet.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 939bec5c8..f328a6637 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -207,6 +207,7 @@ void *dp_packet_resize_l2(struct dp_packet *, int
increment);
void *dp_packet_resize_l2_5(struct dp_packet *, int increment);
static inline void *dp_packet_eth(const struct dp_packet *);
static inline void dp_packet_reset_offsets(struct dp_packet *);
+static inline void dp_packet_reset_offload(struct dp_packet *);
static inline uint16_t dp_packet_l2_pad_size(const struct dp_packet *);
static inline void dp_packet_set_l2_pad_size(struct dp_packet *, uint16_t);
static inline void *dp_packet_l2_5(const struct dp_packet *);
@@ -380,6 +381,7 @@ dp_packet_clear(struct dp_packet *b)
{
dp_packet_set_data(b, dp_packet_base(b));
dp_packet_set_size(b, 0);
+dp_packet_reset_offload(b);
}
/* Removes 'size' bytes from the head end of 'b', which must contain at least
@@ -537,7 +539,7 @@ dp_packet_inner_l4(const struct dp_packet *b)
static inline size_t
dp_packet_inner_l4_size(const struct dp_packet *b)
{
-return OVS_LIKELY(b->l4_ofs != UINT16_MAX)
+return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX)
? (const char *) dp_packet_tail(b)
- (const char *) dp_packet_inner_l4(b)
- dp_packet_l2_pad_size(b)
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] userspace: Check for inner L3 while preparing encapsulated packets.
On Mon, Jan 22, 2024 at 4:10 PM Ilya Maximets wrote:
>
> On 1/22/24 21:33, Mike Pattrick wrote:
> > On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets wrote:
> >>
> >> On 1/22/24 18:51, Mike Pattrick wrote:
> >>> The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
> >>> a double encapsulated BFD packet would trigger a seg fault. This
> >>> happened because we had assumed that if IP checksumming was marked as
> >>> offloaded, that checksum would occur in the innermost packet.
> >>>
> >>> This change will check that the inner packet has an L3 and L4 before
> >>> checksumming those parts. And if missing, will resume checksumming the
> >>> outer components.
> >>
> >> Hrm. This looks like a workaround rather than a fix. If the inner packet
> >> doesn't have L3 header, dp-packet must not have a flag for L3 checksumming
> >> set in the first place. And if it does have inner L3, the offset must be
> >> initialized. I guess, some of the offsets can be not initialized, because
> >> the packet is never parsed by either miniflow_extract() or
> >> parse_tcp_flags().
> >> And the bfd_put_packet() doesn't seem to set them.
> >
> > I think you're right, I stepped through the problem in GDB and it was
> > clear that the flags weren't getting reset properly between BFD
> > packets. I'll send an updated patch.
>
> Yeah, flags preservation is one thing, the other is that l3/l4_ofs are just
> not populated in these packets, which doesn't sound right.
>
> It's also not clear why the packet is marked for inner checksum offload if
> the inner checksum is actually fully calculated and correct. I understand
> that the flags are carried over from a previous packet, but why did that
> previous packet have this flag set?
Here's an example:
monitor_run()
\_ dp_packet_use_stub(, stub, sizeof stub); <--- initializes packet once
\_ while(!heap_is_empty(_heap) <-- loop where the same packet
can be reused
\_ monitor_mport_run()
\_ dp_packet_clear() <- Data cleared, but flags are not cleared
\_ cfm_compose_ccm() / bfd_put_packet() / lldp_put_packet() <-
one or more can run
\_ Note that cfm_compose_ccm and lldp_put_packet call
eth_compose, but bfd_put_packet doesn't. bfd_put_packet doesn't reset
the offsets like eth_compose does.
\_ ofproto_dpif_send_packet()
non-relevant stack trace ...
\_ netdev_push_header()
\_ First run, push geneve header and toggle geneve flag
\_ Second run, detect geneve header flag, call dp_packet_ol_send_prepare()
Given the above, I think it makes sense to clear the offload flags in
dp_packet_clear().
-M
>
> >
> >> BTW, is there actually a double encapsulation in the original OVN test?
> >> Sounds strange.
> >
> > The problem was exposed in netdev_push_header() in
> > dp_packet_ol_send_prepare(packet, 0);
>
> That's true, but the packet dump in gdb showed a plain BFD packet.
> So, it was a first encapsulation, not double.
>
> >
> > -M
> >
> >>
> >>>
> >>> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
> >>> Reported-by: Dumitru Ceara
> >>> Reported-at: https://issues.redhat.com/browse/FDP-300
> >>> Signed-off-by: Mike Pattrick
> >>> ---
> >>> lib/dp-packet.h | 10 --
> >>> lib/packets.c | 6 +++---
> >>> 2 files changed, 11 insertions(+), 5 deletions(-)
> >>
> >> Best regards, Ilya Maximets.
> >>
> >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] userspace: Check for inner L3 while preparing encapsulated packets.
On 1/22/24 21:33, Mike Pattrick wrote:
> On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets wrote:
>>
>> On 1/22/24 18:51, Mike Pattrick wrote:
>>> The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
>>> a double encapsulated BFD packet would trigger a seg fault. This
>>> happened because we had assumed that if IP checksumming was marked as
>>> offloaded, that checksum would occur in the innermost packet.
>>>
>>> This change will check that the inner packet has an L3 and L4 before
>>> checksumming those parts. And if missing, will resume checksumming the
>>> outer components.
>>
>> Hrm. This looks like a workaround rather than a fix. If the inner packet
>> doesn't have L3 header, dp-packet must not have a flag for L3 checksumming
>> set in the first place. And if it does have inner L3, the offset must be
>> initialized. I guess, some of the offsets can be not initialized, because
>> the packet is never parsed by either miniflow_extract() or parse_tcp_flags().
>> And the bfd_put_packet() doesn't seem to set them.
>
> I think you're right, I stepped through the problem in GDB and it was
> clear that the flags weren't getting reset properly between BFD
> packets. I'll send an updated patch.
Yeah, flags preservation is one thing, the other is that l3/l4_ofs are just
not populated in these packets, which doesn't sound right.
It's also not clear why the packet is marked for inner checksum offload if
the inner checksum is actually fully calculated and correct. I understand
that the flags are carried over from a previous packet, but why did that
previous packet have this flag set?
>
>> BTW, is there actually a double encapsulation in the original OVN test?
>> Sounds strange.
>
> The problem was exposed in netdev_push_header() in
> dp_packet_ol_send_prepare(packet, 0);
That's true, but the packet dump in gdb showed a plain BFD packet.
So, it was a first encapsulation, not double.
>
> -M
>
>>
>>>
>>> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
>>> Reported-by: Dumitru Ceara
>>> Reported-at: https://issues.redhat.com/browse/FDP-300
>>> Signed-off-by: Mike Pattrick
>>> ---
>>> lib/dp-packet.h | 10 --
>>> lib/packets.c | 6 +++---
>>> 2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> Best regards, Ilya Maximets.
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] system-common-macros: Fix conntrack matching.
On 1/19/24 14:01, Eelco Chaudron wrote:
>
>
> On 19 Jan 2024, at 13:53, David Marchand wrote:
>
>> On Fri, Jan 19, 2024 at 1:49 PM Ilya Maximets wrote:
>>>
>>> On 1/18/24 14:00, David Marchand wrote:
Seen in GHA recently.
Unit tests are checking conntracks relating to a destination ip address
but the FORMAT_CT macro is not strict enough and would match unrelated
conntracks too.
Example:
148. system-traffic.at:6432: testing conntrack - DNAT with
additional SNAT ...
[...]
./system-traffic.at:6460: ovs-appctl dpctl/dump-conntrack |
grep "dst=10.1.1.1" |
sed -e 's/port=[0-9]*/port=/g'
-e 's/id=[0-9]*/id=/g'
-e 's/state=[0-9_A-Z]*/state=/g' | sort | uniq
[...]
@@ -1,2 +1,7 @@
tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=,...
+tcp,...,reply=(src=13.107.42.16,dst=10.1.1.10,sport=,...
+tcp,...,reply=(src=168.63.129.16,dst=10.1.1.10,sport=,...
+tcp,...,reply=(src=20.242.161.191,dst=10.1.1.10,sport=,...
+tcp,orig=(src=13.107.42.16,dst=10.1.1.10,sport=,...
+tcp,orig=(src=20.242.161.191,dst=10.1.1.10,sport=,...
Fixes: 07659514c3c1 ("Add support for connection tracking.")
Signed-off-by: David Marchand
---
tests/system-common-macros.at | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 01ebe364ee..07be29f673 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -256,7 +256,7 @@ m4_define([STRIP_MONITOR_CSUM], [grep "csum:" | sed
's/csum:.*/csum: /'])
# and limit the output to the rows containing 'ip-addr'.
#
m4_define([FORMAT_CT],
-[[grep "dst=$1" | sed -e 's/port=[0-9]*/port=/g' -e
's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
sort | uniq]])
+[[grep "dst=$1\>" | sed -e 's/port=[0-9]*/port=/g' -e
's/id=[0-9]*/id=/g' -e 's/state=[0-9_A-Z]*/state=/g' |
sort | uniq]])
# NETNS_DAEMONIZE([namespace], [command], [pidfile])
#
>>>
>>> I remembered why the macro is loose. We wanted to be able
>>> to match on "subnets" by supplying only part of the address.
>>>
>>> There was at least one test that used this functionality.
>>> Eelco removed it though here:
>>>
>>> https://github.com/openvswitch/ovs/commit/a80883f7682158c7a6955360ee852e8279f748e9
>>>
>>> Did you check if have any more instances of such tests?
>>
>> I did not.
>>
>>> They can be tricky to find, as we can supply 10.1.1.2 in order
>>> to match 10.1.1.240, for example.
>>
>> Ok, you can discard my patch.
>> Thanks.
>
> But looking at most of the test cases when they put in an IP they
> mean that specific IP not 10.1.1.20?
The key word here is 'most', it's hard to tell for all of them without
actually analyzing the logic of each of them.
> But maybe your NS idea works better.
It should fix the issue at hands. So I marked this one as rejected for
now. We can revisit it later if the need arises.
If we move to separate namespaces per test it may be possible to just
remove filtering entirely and check the exact content of conntrack
tables instead, I hope.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests ovsdb-server: Fix config-file same schema test.
On 1/22/24 12:37, Simon Horman wrote:
> On Sat, Jan 20, 2024 at 09:42:52AM +, Frode Nordahl wrote:
>> When a configuration file is used the ovsdb-server (re-)configures
>> databases in multiple passes. First the configuration file is
>> read and a shash is populated, second the shash is iterated over
>> to remove/create databases.
>>
>> The "ovsdb-server config-file - same schema" test currently relies
>> on a certain ordering of this shash, but we can't really rely on a
>> specific ordering as it would be environment specific.
>>
>> The test currently fails on big endian systems such as s390x with:
>> -WARN|failed to open database 'db2': ovsdb error: ordinals: duplicate
>> database name
>> +WARN|failed to open database 'db': ovsdb error: ordinals: duplicate
>> database name
>>
>> Normalize the logged database name so that the test can focus on
>> the fact that duplication is detected rather than in which order.
>>
>> Fixes: 55140090e63a ("ovsdb-server: Allow user-provided config files.")
>> Signed-off-by: Frode Nordahl
>
> Acked-by: Simon Horman
>
Thanks, Frode and Simon!
Ideally we would add a new argument for the sed strings to the macro
and have a particular replacement logic in the test itself. But since
the replacement is actually only needed for one test, seems fine as-is.
Applied. Backported to 3.3 as well.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] userspace: Check for inner L3 while preparing encapsulated packets.
On Mon, Jan 22, 2024 at 1:47 PM Ilya Maximets wrote:
>
> On 1/22/24 18:51, Mike Pattrick wrote:
> > The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
> > a double encapsulated BFD packet would trigger a seg fault. This
> > happened because we had assumed that if IP checksumming was marked as
> > offloaded, that checksum would occur in the innermost packet.
> >
> > This change will check that the inner packet has an L3 and L4 before
> > checksumming those parts. And if missing, will resume checksumming the
> > outer components.
>
> Hrm. This looks like a workaround rather than a fix. If the inner packet
> doesn't have L3 header, dp-packet must not have a flag for L3 checksumming
> set in the first place. And if it does have inner L3, the offset must be
> initialized. I guess, some of the offsets can be not initialized, because
> the packet is never parsed by either miniflow_extract() or parse_tcp_flags().
> And the bfd_put_packet() doesn't seem to set them.
I think you're right, I stepped through the problem in GDB and it was
clear that the flags weren't getting reset properly between BFD
packets. I'll send an updated patch.
> BTW, is there actually a double encapsulation in the original OVN test?
> Sounds strange.
The problem was exposed in netdev_push_header() in
dp_packet_ol_send_prepare(packet, 0);
-M
>
> >
> > Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
> > Reported-by: Dumitru Ceara
> > Reported-at: https://issues.redhat.com/browse/FDP-300
> > Signed-off-by: Mike Pattrick
> > ---
> > lib/dp-packet.h | 10 --
> > lib/packets.c | 6 +++---
> > 2 files changed, 11 insertions(+), 5 deletions(-)
>
> Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ci: Run system tests in a separate namespace.
On Mon, Jan 22, 2024 at 06:23:06PM +0100, Ilya Maximets wrote: > GitHub runners use 10.1.0.0/16 network as their base network for eth0 > interface. That is causing random system test failures when unexpected > conntrack entries for this network are present, because our system > tests are mainly using 10.1.1.0/24 subnet for their test networks. > > Run system tests in their own network namespace to avoid any unwanted > interference. > > Ideally, we would run every single test in its own namespace, but that > is not a trivial change and will likely be hard to backport. Still > worth investigating in the future. > > Note: Layer3 tunnel tests with Bareudp ports rely on loopback to work, > but lo interface is down by default in new namespaces. So, bringing > it up. These tests are skipped in Ubuntu 22.04, because it doesn't > have bareudp support, but it's better to have the change anyway, so it > doesn't bite us in the future while upgrading the base image. > > Signed-off-by: Ilya Maximets Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ci: Run system tests in a separate namespace.
On 22 Jan 2024, at 18:23, Ilya Maximets wrote: > GitHub runners use 10.1.0.0/16 network as their base network for eth0 > interface. That is causing random system test failures when unexpected > conntrack entries for this network are present, because our system > tests are mainly using 10.1.1.0/24 subnet for their test networks. > > Run system tests in their own network namespace to avoid any unwanted > interference. > > Ideally, we would run every single test in its own namespace, but that > is not a trivial change and will likely be hard to backport. Still > worth investigating in the future. > > Note: Layer3 tunnel tests with Bareudp ports rely on loopback to work, > but lo interface is down by default in new namespaces. So, bringing > it up. These tests are skipped in Ubuntu 22.04, because it doesn't > have bareudp support, but it's better to have the change anyway, so it > doesn't bite us in the future while upgrading the base image. > > Signed-off-by: Ilya Maximets Applied to main and branch-3.3: * 432a0b935 (origin/master) ci: Run system tests in a separate namespace. * dbb92 (origin/branch-3.3) ci: Run system tests in a separate namespace. //Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [OVN coredump with ssl-ciphers args]
Hi Numan: Ping can we get this in? Also need to apply for branch-23.09. cc @Ales Musil Ali On Thu, Jan 18, 2024 at 9:14 AM aginwala wrote: > Thanks: > > fix: > https://patchwork.ozlabs.org/project/ovn/patch/20240117201258.4033-1-amgin...@gmail.com/ > . > > > Ali > > On Tue, Jan 9, 2024 at 7:14 PM Numan Siddique wrote: > >> >> >> On Tue, Jan 9, 2024, 9:46 PM aginwala wrote: >> >>> So it seems it would be ok to use STREAM_SSL_OPTION_HANDLERS for multiple >>> places not just ovn-controller which covers ssl-ciphers and >>> ssl-protocols. >>> Let me know and can amend in other patch or amend in same patch. >>> >> >> >> Thanks for root causing it. I think one patch is good. >> >> >>> On Tue, Jan 9, 2024 at 4:15 PM aginwala wrote: >>> >>> > Hi : >>> > >>> > Debugging further with gdb, I was able to figure out it was was missed >>> in >>> > ovn-controller part of stream ssl option hanlder >>> > >>> > >>> > git diff >>> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> > index 632a2cb15..66316e057 100644 >>> > --- a/controller/ovn-controller.c >>> > +++ b/controller/ovn-controller.c >>> > @@ -6191,6 +6191,13 @@ parse_options(int argc, char *argv[]) >>> > ssl_ca_cert_file = optarg; >>> > break; >>> > >>> > +case OPT_SSL_PROTOCOLS: >>> > +stream_ssl_set_protocols(optarg); >>> > +break; >>> > + >>> > +case OPT_SSL_CIPHERS: >>> > +stream_ssl_set_ciphers(optarg); >>> > +break; >>> > >>> > case OPT_PEER_CA_CERT: >>> > stream_ssl_set_peer_ca_cert_file(optarg); >>> > >>> > Works fine after compiling with this fix. I can send a formal pr >>> > accordingly. >>> > >>> > >>> > Regards, >>> > Ali >>> > >>> > On Mon, Jan 8, 2024 at 3:35 PM aginwala wrote: >>> > >>> >> Hi: >>> >> >>> >> When setting extra args like ssl-cipers for ovn-controller, it >>> results in >>> >> coredump on branch 23.09 >>> >> compiled with --with-ovs-source and --with-ovs-build option, OVS >>> >> (branch-3.2) >>> >> >>> >> dump: >>> >> Using host libthread_db library >>> "/lib/x86_64-linux-gnu/libthread_db.so.1". >>> >> Core was generated by `ovn-controller >>> >> --ssl-ciphers=HIGH:!aNULL:!MD5:@SECLEVEL=1 unix:/var/run/openvsw' >>> >> Program terminated with signal SIGABRT, Aborted. >>> >> #0 __GI_raise (sig=sig@entry=6) at >>> ../sysdeps/unix/sysv/linux/raise.c:50 >>> >> 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. >>> >> (gdb) bt >>> >> #0 __GI_raise (sig=sig@entry=6) at >>> ../sysdeps/unix/sysv/linux/raise.c:50 >>> >> #1 0x7fb13d7cf859 in __GI_abort () at abort.c:79 >>> >> #2 0x563257fac75c in main (argc=, argv=>> >> out>) at controller/ovn-controller.c:6019 >>> >> (gdb) frame 2 >>> >> #2 0x563257fac75c in main (argc=, argv=>> >> out>) at controller/ovn-controller.c:6019 >>> >> 6019OVS_NOT_REACHED(); >>> >> (gdb) quit >>> >> >>> >> ##ovn-controller --version >>> >> ovn-controller 23.09.1 >>> >> Open vSwitch Library 3.2.2 >>> >> OpenFlow versions 0x6:0x6 >>> >> SB DB Schema 20.29.0 >>> >> >>> >> ##Same happens even on trying with any ovn-* commands >>> >> ovn-nbctl --ssl-ciphers='xx' >>> >> Aborted (core dumped) >>> >> ovn-nbctl --version >>> >> ovn-nbctl 23.09.1 >>> >> Open vSwitch Library 3.2.2 >>> >> DB Schema 7.1.0 >>> >> >>> >> ## back trace for ovn-nbctl >>> >> #0 __GI_raise (sig=sig@entry=6) at >>> ../sysdeps/unix/sysv/linux/raise.c:50 >>> >> 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. >>> >> (gdb) frame 2 >>> >> #2 0x562c759485aa in apply_options_direct >>> >> (local_options=0x7ffcaa25fbb0, n=1, parsed_options=, >>> >> dbctl_options=0x7ffcaa25fc40) at utilities/ovn-dbctl.c:621 >>> >> 621OVS_NOT_REACHED(); >>> >> >>> >> --ssl-ciphers works fine when using ovn 20.03 ; directly using ovn >>> debian >>> >> ovn-controller 20.03.2 >>> >> Open vSwitch Library 2.13.8 >>> >> OpenFlow versions 0x4:0x4 >>> >> SB DB Schema 2.7.0 >>> >> >>> >> ## underlying ovs >>> >> ~/ovn# ovs-vsctl --version >>> >> ovs-vsctl (Open vSwitch) 2.16.8 >>> >> DB Schema 8.3.0 >>> >> >>> >> #Kernel/distio: >>> >> 5.4.0-167-generic/Ubuntu 20.04.6 LTS >>> >> >>> >> >>> >> To avoid invalidating certs on already running computes setup with old >>> >> ovs pki infra, setting ciphers to HIGH:!aNULL:!MD5:@SECLEVEL=1 works >>> >> fine part of bumping to newer 20.x and avoid connectivity failures to >>> >> control plane due mostly due to below error. >>> >> SSL_connect: error:1416F086:SSL >>> >> routines:tls_process_server_certificate:certificate verify failed >>> while >>> >> connecting to control plane. >>> >> >>> >> >>> >> Not sure if it's a known issue with newer OVS stream-ssl. Core file >>> >> attached. >>> >> >>> >> >>> >> Regards, >>> >> Ali >>> >> >>> > >>> ___ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>
Re: [ovs-dev] [PATCH] ci: Run system tests in a separate namespace.
On 22 Jan 2024, at 18:23, Ilya Maximets wrote: > GitHub runners use 10.1.0.0/16 network as their base network for eth0 > interface. That is causing random system test failures when unexpected > conntrack entries for this network are present, because our system > tests are mainly using 10.1.1.0/24 subnet for their test networks. > > Run system tests in their own network namespace to avoid any unwanted > interference. > > Ideally, we would run every single test in its own namespace, but that > is not a trivial change and will likely be hard to backport. Still > worth investigating in the future. > > Note: Layer3 tunnel tests with Bareudp ports rely on loopback to work, > but lo interface is down by default in new namespaces. So, bringing > it up. These tests are skipped in Ubuntu 22.04, because it doesn't > have bareudp support, but it's better to have the change anyway, so it > doesn't bite us in the future while upgrading the base image. > > Signed-off-by: Ilya Maximets Thanks Ilya for posting this patch. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] userspace: Check for inner L3 while preparing encapsulated packets.
On 1/22/24 18:51, Mike Pattrick wrote:
> The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
> a double encapsulated BFD packet would trigger a seg fault. This
> happened because we had assumed that if IP checksumming was marked as
> offloaded, that checksum would occur in the innermost packet.
>
> This change will check that the inner packet has an L3 and L4 before
> checksumming those parts. And if missing, will resume checksumming the
> outer components.
Hrm. This looks like a workaround rather than a fix. If the inner packet
doesn't have L3 header, dp-packet must not have a flag for L3 checksumming
set in the first place. And if it does have inner L3, the offset must be
initialized. I guess, some of the offsets can be not initialized, because
the packet is never parsed by either miniflow_extract() or parse_tcp_flags().
And the bfd_put_packet() doesn't seem to set them.
BTW, is there actually a double encapsulation in the original OVN test?
Sounds strange.
>
> Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
> Reported-by: Dumitru Ceara
> Reported-at: https://issues.redhat.com/browse/FDP-300
> Signed-off-by: Mike Pattrick
> ---
> lib/dp-packet.h | 10 --
> lib/packets.c | 6 +++---
> 2 files changed, 11 insertions(+), 5 deletions(-)
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] userspace: Check for inner L3 while preparing encapsulated packets.
The OVN test suite identified a bug in dp_packet_ol_send_prepare() where
a double encapsulated BFD packet would trigger a seg fault. This
happened because we had assumed that if IP checksumming was marked as
offloaded, that checksum would occur in the innermost packet.
This change will check that the inner packet has an L3 and L4 before
checksumming those parts. And if missing, will resume checksumming the
outer components.
Fixes: 8b5fe2dc6080 ("userspace: Add Generic Segmentation Offloading.")
Reported-by: Dumitru Ceara
Reported-at: https://issues.redhat.com/browse/FDP-300
Signed-off-by: Mike Pattrick
---
lib/dp-packet.h | 10 --
lib/packets.c | 6 +++---
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 939bec5c8..81b01f331 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -537,7 +537,7 @@ dp_packet_inner_l4(const struct dp_packet *b)
static inline size_t
dp_packet_inner_l4_size(const struct dp_packet *b)
{
-return OVS_LIKELY(b->l4_ofs != UINT16_MAX)
+return OVS_LIKELY(b->inner_l4_ofs != UINT16_MAX)
? (const char *) dp_packet_tail(b)
- (const char *) dp_packet_inner_l4(b)
- dp_packet_l2_pad_size(b)
@@ -1385,7 +1385,13 @@ dp_packet_ip_checksum_bad(const struct dp_packet *p)
static inline void
dp_packet_ip_set_header_csum(struct dp_packet *p, bool inner)
{
-struct ip_header *ip = (inner) ? dp_packet_inner_l3(p) : dp_packet_l3(p);
+struct ip_header *ip;
+
+if (inner && dp_packet_inner_l3(p)) {
+ip = dp_packet_inner_l3(p);
+} else {
+ip = dp_packet_l3(p);
+}
ovs_assert(ip);
ip->ip_csum = 0;
diff --git a/lib/packets.c b/lib/packets.c
index f23d25420..921862b0d 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -2004,7 +2004,7 @@ packet_tcp_complete_csum(struct dp_packet *p, bool inner)
void *ip_hdr;
bool is_v4;
-if (inner) {
+if (inner && dp_packet_inner_l4(p)) {
tcp = dp_packet_inner_l4(p);
ip_hdr = dp_packet_inner_l3(p);
tcp_sz = dp_packet_inner_l4_size(p);
@@ -2050,7 +2050,7 @@ packet_udp_complete_csum(struct dp_packet *p, bool inner)
void *ip_hdr;
bool is_v4;
-if (inner) {
+if (inner && dp_packet_inner_l4(p)) {
udp = dp_packet_inner_l4(p);
ip_hdr = dp_packet_inner_l3(p);
udp_sz = dp_packet_inner_l4_size(p);
@@ -2104,7 +2104,7 @@ packet_sctp_complete_csum(struct dp_packet *p, bool inner)
uint16_t tp_len;
ovs_be32 csum;
-if (inner) {
+if (inner && dp_packet_inner_l4(p)) {
sh = dp_packet_inner_l4(p);
tp_len = dp_packet_inner_l4_size(p);
} else {
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] compiler: Fix errors in Clang 17 ubsan checks.
On Fri, Jan 19, 2024 at 02:02:04PM -0500, Mike Pattrick wrote: > The github ci failed with entries like this in the logs: > > +tcp,orig=(src=10.1.1.4,dst=140.82.113.22,sport=,dport=),reply=(src=140.82.113.22,dst=10.1.1.4,sport=,dport=),protoinfo=(state=) > +tcp,orig=(src=10.1.1.4,dst=168.63.129.16,sport=,dport=),reply=(src=168.63.129.16,dst=10.1.1.4,sport=,dport=),protoinfo=(state=) > +tcp,orig=(src=10.1.1.4,dst=20.22.166.15,sport=,dport=),reply=(src=20.22.166.15,dst=10.1.1.4,sport=,dport=),protoinfo=(state=) > > Those are github IP's, don't know how github traffic got into the tests. Ilya says that this is because "GitHub runners use 10.1.0.0/16 network as their base network for eth0 interface. That is causing random system test failures when unexpected conntrack entries for this network are present, because our system tests are mainly using 10.1.1.0/24 subnet for their test networks. The link below is for a proposed fix for this problem. [1] https://patchwork.ozlabs.org/project/openvswitch/patch/20240122172307.3801928-1-i.maxim...@ovn.org/ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ci: Run system tests in a separate namespace.
GitHub runners use 10.1.0.0/16 network as their base network for eth0
interface. That is causing random system test failures when unexpected
conntrack entries for this network are present, because our system
tests are mainly using 10.1.1.0/24 subnet for their test networks.
Run system tests in their own network namespace to avoid any unwanted
interference.
Ideally, we would run every single test in its own namespace, but that
is not a trivial change and will likely be hard to backport. Still
worth investigating in the future.
Note: Layer3 tunnel tests with Bareudp ports rely on loopback to work,
but lo interface is down by default in new namespaces. So, bringing
it up. These tests are skipped in Ubuntu 22.04, because it doesn't
have bareudp support, but it's better to have the change anyway, so it
doesn't bite us in the future while upgrading the base image.
Signed-off-by: Ilya Maximets
---
.ci/linux-build.sh | 4
1 file changed, 4 insertions(+)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 7c2aebad8..bf9d6241d 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -157,6 +157,10 @@ else
if [ "$testsuite" != "check" ] && \
[ "$testsuite" != "check-ovsdb-cluster" ] ; then
run_as_root="sudo -E PATH=$PATH GITHUB_ACTIONS=$GITHUB_ACTIONS"
+sudo ip netns add ovs-system-test-ns
+# Some system tests may rely on traffic loopback.
+sudo ip -netns ovs-system-test-ns link set dev lo up
+run_as_root="${run_as_root} ip netns exec ovs-system-test-ns"
fi
if [ "${testsuite##*dpdk}" != "$testsuite" ]; then
sudo sh -c 'echo 1024 > /proc/sys/vm/nr_hugepages' || true
--
2.43.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH OVN] Add "disable_arp_nd_rsp" option to LSP
This option can be used to enable/disable arp/nd reply flows.
Usecase:
=
It is useful to reduce packet loss when VM is being migrated to
different AZ via VXLAN tunnel. Port is configured in both AZs
on different logical switches which are sharing same IP subnet.
In reality, the port is active on only one logical switch.
Skipping ARP/ND responder and letting the ARP/ND get flooded to
learn the location of the port.
Signed-off-by: Naveen Yerramneni
---
northd/northd.c | 10 +-
tests/ovn-northd.at | 31 +++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/northd/northd.c b/northd/northd.c
index 952f8200d..4e070c0fe 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -1844,6 +1844,12 @@ localnet_can_learn_mac(const struct
nbrec_logical_switch_port *nbsp)
return smap_get_bool(>options, "localnet_learn_fdb", false);
}
+static bool
+lsp_disable_arp_nd_rsp(const struct nbrec_logical_switch_port *nbsp)
+{
+return smap_get_bool(>options, "disable_arp_nd_rsp", false);
+}
+
static bool
lsp_is_type_changed(const struct sbrec_port_binding *sb,
const struct nbrec_logical_switch_port *nbsp,
@@ -9921,7 +9927,9 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port
*op,
return;
}
-if (lsp_is_external(op->nbsp) || op->has_unknown) {
+if (lsp_is_external(op->nbsp) || op->has_unknown ||
+ (!strcmp(op->nbsp->type, "") &&
+lsp_disable_arp_nd_rsp(op->nbsp))) {
return;
}
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 9a0d418e4..9a36ee810 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -11094,5 +11094,36 @@ AT_CHECK([ovn-sbctl dump-flows S1 | grep pre_acl | sed
's/table=./table=?/'], [0
])
+AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check options:disable_arp_nd_rsp for LSP])
+ovn_start NORTHD_TYPE
+ovn-nbctl ls-add S1
+ovn-nbctl --wait=sb lsp-add S1 S1-vm1
+ovn-nbctl --wait=sb lsp-set-addresses S1-vm1 "50:54:00:00:00:010 192.168.0.10
fd00::10"
+
+ovn-sbctl dump-flows S1 > S1flows
+AT_CAPTURE_FILE([S1flows])
+
+AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | sed 's/table=../table=??/'], [0],
[dnl
+ table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa ==
192.168.0.10 && arp.op == 1 && inport == "S1-vm1"), action=(next;)
+ table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns && ip6.dst ==
{fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"),
action=(next;)
+ table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa ==
192.168.0.10 && arp.op == 1), action=(eth.dst = eth.src; eth.src =
50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha =
50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport = inport;
flags.loopback = 1; output;)
+ table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst ==
{fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src
= 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll =
50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };)
+ table=??(ls_in_arp_rsp ), priority=0, match=(1), action=(next;)
+])
+
+#Set the disable_arp_nd_rsp option and verify the flow
+ovn-nbctl --wait=sb set logical_switch_port S1-vm1
options:disable_arp_nd_rsp=true
+
+ovn-sbctl dump-flows S1 > S1flows
+AT_CAPTURE_FILE([S1flows])
+
+AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | sed 's/table=../table=??/'], [0],
[dnl
+ table=??(ls_in_arp_rsp ), priority=0, match=(1), action=(next;)
+])
+
AT_CLEANUP
])
--
2.36.6
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH OVN] Add "pkt_clone_type" option to LSP.
Currently only valid value for this option is mc_unknown.
If set to mc_unknown, packets destined to the port gets cloned
to all unknown ports connected to the same Logical Switch.
Usecase:
=
It is required to reduce packet loss when VM is being migrated to
different AZ via VXLAN tunnel. Port is configured in both AZs
on different logical switches which are sharing same IP subnet.
VTEP device is connected to the same logical switch on both AZs
and its port is set to unknown.
Signed-off-by: Naveen Yerramneni
---
northd/northd.c | 23 +--
ovn-nb.xml | 6 ++
tests/ovn-northd.at | 32
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index 952f8200d..98e97fd6b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -1786,6 +1786,17 @@ ovn_port_find(const struct hmap *ports, const char *name)
return ovn_port_find__(ports, name, false);
}
+static bool
+lsp_is_clone_to_unknown(const struct nbrec_logical_switch_port *nbsp)
+{
+const char *pkt_clone_type = smap_get(>options,
+ "pkt_clone_type");
+if (pkt_clone_type && !strcasecmp(pkt_clone_type, "mc_unknown")) {
+return true;
+}
+return false;
+}
+
static struct ovn_port *
ovn_port_find_bound(const struct hmap *ports, const char *name)
{
@@ -10529,8 +10540,16 @@ build_lswitch_ip_unicast_lookup(struct ovn_port *op,
* or IPv6 addresses (or both). */
struct eth_addr mac;
bool lsp_enabled = lsp_is_enabled(op->nbsp);
-const char *action = lsp_enabled ? "outport = %s; output;" :
- debug_drop_action();
+bool lsp_clone_to_unknown = lsp_is_clone_to_unknown(op->nbsp);
+const char *action = lsp_enabled ?
+((lsp_clone_to_unknown && op->od->has_unknown
+ && !strcmp(op->nbsp->type, "")) ?
+ "clone {outport = %s; output; };"
+ "outport = \""MC_UNKNOWN "\"; output;" :
+ "outport = %s; output;") :
+ debug_drop_action();
+
+
if (ovs_scan(op->nbsp->addresses[i],
ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
ds_clear(match);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 765ffcf2e..dc55c8c99 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1262,6 +1262,12 @@
+
+ If set to mc_unknown, packets going to this VIF get cloned to all
+ unknown ports connected to the same Logical Switch.
+
+
If set, OVN will attempt to perform plugging of this VIF. In order
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 9a0d418e4..0ca9f82bb 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -6650,6 +6650,38 @@ wait_row_count Port_binding 1 logical-port=S1-vm2
requested_chassis=$ch2_uuid
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check options:pkt_clone_type for LSP])
+ovn_start NORTHD_TYPE
+ovn-nbctl ls-add S1
+ovn-nbctl --wait=sb lsp-add S1 S1-vm1
+ovn-nbctl --wait=sb lsp-add S1 S1-vm2
+ovn-nbctl --wait=sb lsp-set-addresses S1-vm1 "50:54:00:00:00:01 192.168.0.1"
+ovn-nbctl --wait=sb lsp-set-addresses S1-vm2 "unknown"
+
+ovn-sbctl dump-flows S1 > S1flows
+AT_CAPTURE_FILE([S1flows])
+
+AT_CHECK([grep -e "ls_in_l2_lkup.*S1-vm1" S1flows | sed
's/table=../table=??/'], [0], [dnl
+ table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst ==
50:54:00:00:00:01), action=(outport = "S1-vm1"; output;)
+])
+
+#Set the pkt_clone_type option and verify the flow
+ovn-nbctl --wait=sb set logical_switch_port S1-vm1
options:pkt_clone_type=mc_unknown
+
+ovn-sbctl dump-flows S1 > S1flows
+AT_CAPTURE_FILE([S1flows])
+
+AT_CHECK([grep -e "ls_in_l2_lkup.*S1-vm1" S1flows | sed
's/table=../table=??/'], [0], [dnl
+ table=??(ls_in_l2_lkup ), priority=50 , match=(eth.dst ==
50:54:00:00:00:01), action=(clone {outport = "S1-vm1"; output; };outport =
"_MC_unknown"; output;)
+])
+
+AT_CLEANUP
+])
+
+
+
# Duplicated datapaths shouldn't be created, but in case it is created because
# of bug or dirty data, it should be properly deleted instead of causing
# permanent failure in northd.
--
2.36.6
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 2/2] OVN-SB: Exposes igmp group protocol version through IGMP table.
On 1/22/24 15:14, Mohammad Heib wrote:
> Expose the igmp/mld group protocol version through the
> IGMP_GROUP table in SBDB.
>
> This patch can be used by ovn consumer for debuggability purposes, user
> now can match between the protocol version used in the OVN logical
> switches and the uplink ports.
>
> Rreported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160476
> Signed-off-by: Mohammad Heib
> ---
Hi Mohammad,
I have a few (minor) comments, please see below.
> NEWS | 2 ++
> controller/ip-mcast.c | 8
> controller/ip-mcast.h | 3 +++
> controller/pinctrl.c | 19 ++-
> northd/ovn-northd.c | 2 +-
> ovn-sb.ovsschema | 5 +++--
> ovn-sb.xml| 4
> tests/ovn.at | 3 +++
> 8 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 5f267b4c6..9075e7d80 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,8 @@ Post v23.09.0
>- ovn-northd-ddlog has been removed.
>- A new LSP option "enable_router_port_acl" has been added to enable
> conntrack for the router port whose peer is l3dgw_port if set it true.
> + - IGMP_Group have a new "protocol" column that displays the the group
Nit: s/have/has
> +protocol version.
>
> OVN v23.09.0 - 15 Sep 2023
> --
> diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c
> index a870fb29e..83e41c81f 100644
> --- a/controller/ip-mcast.c
> +++ b/controller/ip-mcast.c
> @@ -226,6 +226,14 @@ igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> return true;
> }
>
> +
> +void igmp_group_set_protocol(const struct sbrec_igmp_group *group,
> + mcast_group_proto protocol)
> +{
> +sbrec_igmp_group_set_protocol(group,
> +
> mcast_snooping_group_protocol_str(protocol));
> +}
> +
Does it make more sense to add the protocol as argument to
igmp_group_update_ports() and rename that function to igmp_group_update()?
> static const struct sbrec_igmp_group *
> igmp_group_lookup_(struct ovsdb_idl_index *igmp_groups,
> const char *addr_str,
> diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
> index 326f39db1..f0c34343f 100644
> --- a/controller/ip-mcast.h
> +++ b/controller/ip-mcast.h
> @@ -63,4 +63,7 @@ void igmp_group_delete(const struct sbrec_igmp_group *g);
> bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *igmp_groups);
>
> +void igmp_group_set_protocol(const struct sbrec_igmp_group *group,
> + mcast_group_proto protocol);
> +
> #endif /* controller/ip-mcast.h */
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 77bf67e58..6379b7afb 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -180,6 +180,7 @@ struct pinctrl {
> bool mac_binding_can_timestamp;
> bool fdb_can_timestamp;
> bool dns_supports_ovn_owned;
> +bool igmp_support_protocol;
> };
>
> static struct pinctrl pinctrl;
> @@ -3586,11 +3587,21 @@ pinctrl_update(const struct ovsdb_idl *idl, const
> char *br_int_name)
> if (dns_supports_ovn_owned != pinctrl.dns_supports_ovn_owned) {
> pinctrl.dns_supports_ovn_owned = dns_supports_ovn_owned;
>
> -/* Notify pinctrl_handler that fdb timestamp column
> +/* Notify pinctrl_handler that dns ovn_owned column
> * availability has changed. */
> notify_pinctrl_handler();
> }
>
> +bool igmp_support_proto =
> +sbrec_server_has_igmp_group_table_col_protocol(idl);
> +if (igmp_support_proto != pinctrl.igmp_support_protocol) {
> +pinctrl.igmp_support_protocol = igmp_support_proto;
We only use this in the main thread, when updating the SB, why can't we
just directly check the column support there instead?
> +
> +/* Notify pinctrl_handler that igmp protocol column
> + * availability has changed. */
> +notify_pinctrl_handler();
> +}
> +
> ovs_mutex_unlock(_mutex);
> }
>
> @@ -5400,6 +5411,12 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
> local_dp->datapath, chassis);
> }
>
> +/* Set Group protocol*/
> +if (pinctrl.igmp_support_protocol) {
> +igmp_group_set_protocol(sbrec_igmp,
> +mc_group->protocol_version);
> +}
> +
> igmp_group_update_ports(sbrec_igmp,
> sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key, ip_ms->ms,
> mc_group);
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index f3868068d..700c9cf6e 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -120,7 +120,7 @@ static const char *rbac_svc_monitor_auth_update[] =
> static const char *rbac_igmp_group_auth[] =
> {""};
Re: [ovs-dev] [PATCH ovn 1/2] ovs: Bump submodule to include igmp protocol version.
On 1/22/24 15:14, Mohammad Heib wrote:
> Specifically the following commit:
> 077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.")
>
> Also fix a small compilation error due to prototype change.
>
> Signed-off-by: Mohammad Heib
> ---
Hi Mohammad,
Thanks for the patch!
> controller/pinctrl.c | 6 +-
> ovs | 2 +-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 4992eab08..77bf67e58 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn,
> switch (ntohs(ip_flow->tp_src)) {
> case IGMP_HOST_MEMBERSHIP_REPORT:
> case IGMPV2_HOST_MEMBERSHIP_REPORT:
> +mcast_group_proto grp_proto =
> +(ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT)
> +? MCAST_GROUP_IGMPV1
> +: MCAST_GROUP_IGMPV2;
> group_change =
> mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN,
> - port_key_data);
> + port_key_data, grp_proto);
> break;
> case IGMP_HOST_LEAVE_MESSAGE:
> group_change =
> diff --git a/ovs b/ovs
> index 4102674b3..b222593bc 16
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6
> +Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3
However, it's probably desirable to bump the submodule to the tip of the
latest stable branch, i.e. branch-3.3:
https://github.com/ovn-org/ovn/blob/main/Documentation/internals/ovs_submodule.rst#submodules-for-releases
That would also fix our scheduled CI runs:
https://github.com/ovn-org/ovn/actions/runs/7597582431/job/20692570222#step:10:3531
However, there's a crash in OVS on branch-3.3 that needs to be fixed first:
https://issues.redhat.com/browse/FDP-300
I'd wait with bumping the submodule until then.
Regards,
Dumitru
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] actions: Use random port selection for SNAT with external_port_range.
This is to avoid unexpected behavior changes due to the underlying
datapath (e.g., kernel) changing defaults. If we don't explicitly
request a port selection algorithm, OVS leaves it up to the
datapath to decide how to do the port selection. Currently that means
that source port allocation is not random if the original source port
fits in the requested range.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2024-January/410847.html
Reported-at: https://issues.redhat.com/browse/FDP-301
Fixes: 60bdc8ea78d7 ("NAT: Provide port range in input")
Signed-off-by: Dumitru Ceara
---
lib/actions.c | 16 ++--
tests/ovn.at | 8
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/lib/actions.c b/lib/actions.c
index 38cf4642d6..fdc0529de6 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1131,8 +1131,20 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
}
if (cn->port_range.exists) {
- nat->range.proto.min = cn->port_range.port_lo;
- nat->range.proto.max = cn->port_range.port_hi;
+nat->range.proto.min = cn->port_range.port_lo;
+nat->range.proto.max = cn->port_range.port_hi;
+
+/* Explicitly set the port selection algorithm to "random". Otherwise
+ * it's up to the datapath to choose how to select the port and that
+ * might create unexpected behavior changes when the datapath defaults
+ * change.
+ *
+ * NOTE: for the userspace datapath the "random" function doesn't
+ * really generate random ports, it uses "hash" under the hood:
+ * https://issues.redhat.com/browse/FDP-269. */
+if (nat->range.proto.min && nat->range.proto.max) {
+nat->flags |= NX_NAT_F_PROTO_RANDOM;
+}
}
ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
diff --git a/tests/ovn.at b/tests/ovn.at
index 8cc4c311c1..e8cef83cb5 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1368,7 +1368,7 @@ ct_dnat(fd11::2);
has prereqs ip
ct_dnat(192.168.1.2, 1-3000);
formats as ct_dnat(192.168.1.2,1-3000);
-encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000))
+encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000,random))
has prereqs ip
ct_dnat(192.168.1.2, 192.168.1.3);
@@ -1402,7 +1402,7 @@ ct_dnat_in_czone(fd11::2);
has prereqs ip
ct_dnat_in_czone(192.168.1.2, 1-3000);
formats as ct_dnat_in_czone(192.168.1.2,1-3000);
-encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000))
+encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(dst=192.168.1.2:1-3000,random))
has prereqs ip
ct_dnat_in_czone(192.168.1.2, 192.168.1.3);
@@ -1436,7 +1436,7 @@ ct_snat(fd11::2);
has prereqs ip
ct_snat(192.168.1.2, 1-3000);
formats as ct_snat(192.168.1.2,1-3000);
-encodes as
ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000))
+encodes as
ct(commit,table=19,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2:1-3000,random))
has prereqs ip
ct_snat(192.168.1.2, 192.168.1.3);
@@ -1470,7 +1470,7 @@ ct_snat_in_czone(fd11::2);
has prereqs ip
ct_snat_in_czone(192.168.1.2, 1-3000);
formats as ct_snat_in_czone(192.168.1.2,1-3000);
-encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(src=192.168.1.2:1-3000))
+encodes as
ct(commit,table=19,zone=NXM_NX_REG11[0..15],nat(src=192.168.1.2:1-3000,random))
has prereqs ip
ct_snat_in_czone(192.168.1.2, 192.168.1.3);
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
On Mon, Jan 22, 2024 at 9:09 AM Felix Huettner via dev <
ovs-dev@openvswitch.org> wrote:
> On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> > With this change, a chassis may only update MAC Binding records that it
> > has created. We achieve this by adding a "chassis_name" column to the
> > MAC_Binding table, and having the chassis insert its name into this
> > column when creating a new MAC_Binding. The "chassis_name" is now part
> > of the rbac_auth structure for the MAC_Binding table.
>
> Hi Mark,
>
> i am concerned that this will negatively impact MAC_Bindings for LRPs
> with multiple gateway chassis.
>
> Suppose a MAC_Binding is first learned by an LRP currently residing on
> chassis1. The LRP then failovers to chassis2 and chassis1 is potentially
> even
> removed completely. In this case the ovn-controller on chassis2 would no
> longer be allowed to update the timestamp column. This would break the
> arp refresh mechanism.
>
> In this case the MAC_Binding would need to expire first, causing northd
> to removed it. Afterwards chassis2 would be allowed to insert a new
> record with its own chassis name.
>
> I honestly did not try out this case so i am not fully sure if this
> issue realy exists or if i have a missunderstanding somewhere.
>
> Thanks
> Felix
>
>
Hi Mark and Felix,
I personally don't see the ability to not refresh as an issue, the MAC
binding would age out and the node could create a new one. However, it will
still produce errors when the remote chassis tries to update the timestamp
of MAC binding owned by someone else.
There is another issue that I'm more concerned about and that's in case the
aging is not enabled at all. After failover the MAC binding might not be
updated at all. Similar issue applies to MAM bindings distributed across
many chassis. One will own it and only that chassis can update MAC address
when anything changes which it might never do.
To solve that we would need duplicates per chassis, basically the same MAC
binding row, but with different "owners". This goes in hand with having OF
only for MAC bindings owned by current chassis and nothing else. Does that
make sense?
All the above unfortunately applies also to FDB.
Thanks,
Ales
> > ---
> > controller/pinctrl.c | 51
> > northd/ovn-northd.c | 2 +-
> > ovn-sb.ovsschema | 7 +++---
> > ovn-sb.xml | 3 +++
> > 4 files changed, 45 insertions(+), 18 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 4992eab08..a00cdceea 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -180,6 +180,7 @@ struct pinctrl {
> > bool mac_binding_can_timestamp;
> > bool fdb_can_timestamp;
> > bool dns_supports_ovn_owned;
> > +bool mac_binding_has_chassis_name;
> > };
> >
> > static struct pinctrl pinctrl;
> > @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> > struct ovsdb_idl_txn *ovnsb_idl_txn,
> > struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> > struct ovsdb_idl_index *sbrec_port_binding_by_key,
> > -struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> > +struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> > +const struct sbrec_chassis *chassis)
> > OVS_REQUIRES(pinctrl_mutex);
> > static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
> > static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> > @@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const
> char *br_int_name)
> > notify_pinctrl_handler();
> > }
> >
> > +bool mac_binding_has_chassis_name =
> > +sbrec_server_has_mac_binding_table_col_chassis_name(idl);
> > +if (mac_binding_has_chassis_name !=
> pinctrl.mac_binding_has_chassis_name) {
> > +pinctrl.mac_binding_has_chassis_name =
> mac_binding_has_chassis_name;
> > +notify_pinctrl_handler();
> > +}
> > +
> > ovs_mutex_unlock(_mutex);
> > }
> >
> > @@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > ovs_mutex_lock(_mutex);
> > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> > sbrec_port_binding_by_key,
> > - sbrec_mac_binding_by_lport_ip);
> > + sbrec_mac_binding_by_lport_ip,
> > + chassis);
> > run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> > sbrec_port_binding_by_key, chassis);
> > send_garp_rarp_prepare(ovnsb_idl_txn,
> sbrec_port_binding_by_datapath,
> > @@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> >const char *logical_port,
> >const struct sbrec_datapath_binding *dp,
> >struct eth_addr ea, const char *ip,
> > - bool update_only)
> > + bool
[ovs-dev] [PATCH ovn 2/2] OVN-SB: Exposes igmp group protocol version through IGMP table.
Expose the igmp/mld group protocol version through the
IGMP_GROUP table in SBDB.
This patch can be used by ovn consumer for debuggability purposes, user
now can match between the protocol version used in the OVN logical
switches and the uplink ports.
Rreported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160476
Signed-off-by: Mohammad Heib
---
NEWS | 2 ++
controller/ip-mcast.c | 8
controller/ip-mcast.h | 3 +++
controller/pinctrl.c | 19 ++-
northd/ovn-northd.c | 2 +-
ovn-sb.ovsschema | 5 +++--
ovn-sb.xml| 4
tests/ovn.at | 3 +++
8 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/NEWS b/NEWS
index 5f267b4c6..9075e7d80 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@ Post v23.09.0
- ovn-northd-ddlog has been removed.
- A new LSP option "enable_router_port_acl" has been added to enable
conntrack for the router port whose peer is l3dgw_port if set it true.
+ - IGMP_Group have a new "protocol" column that displays the the group
+protocol version.
OVN v23.09.0 - 15 Sep 2023
--
diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c
index a870fb29e..83e41c81f 100644
--- a/controller/ip-mcast.c
+++ b/controller/ip-mcast.c
@@ -226,6 +226,14 @@ igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
return true;
}
+
+void igmp_group_set_protocol(const struct sbrec_igmp_group *group,
+ mcast_group_proto protocol)
+{
+sbrec_igmp_group_set_protocol(group,
+ mcast_snooping_group_protocol_str(protocol));
+}
+
static const struct sbrec_igmp_group *
igmp_group_lookup_(struct ovsdb_idl_index *igmp_groups,
const char *addr_str,
diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
index 326f39db1..f0c34343f 100644
--- a/controller/ip-mcast.h
+++ b/controller/ip-mcast.h
@@ -63,4 +63,7 @@ void igmp_group_delete(const struct sbrec_igmp_group *g);
bool igmp_group_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *igmp_groups);
+void igmp_group_set_protocol(const struct sbrec_igmp_group *group,
+ mcast_group_proto protocol);
+
#endif /* controller/ip-mcast.h */
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 77bf67e58..6379b7afb 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -180,6 +180,7 @@ struct pinctrl {
bool mac_binding_can_timestamp;
bool fdb_can_timestamp;
bool dns_supports_ovn_owned;
+bool igmp_support_protocol;
};
static struct pinctrl pinctrl;
@@ -3586,11 +3587,21 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
*br_int_name)
if (dns_supports_ovn_owned != pinctrl.dns_supports_ovn_owned) {
pinctrl.dns_supports_ovn_owned = dns_supports_ovn_owned;
-/* Notify pinctrl_handler that fdb timestamp column
+/* Notify pinctrl_handler that dns ovn_owned column
* availability has changed. */
notify_pinctrl_handler();
}
+bool igmp_support_proto =
+sbrec_server_has_igmp_group_table_col_protocol(idl);
+if (igmp_support_proto != pinctrl.igmp_support_protocol) {
+pinctrl.igmp_support_protocol = igmp_support_proto;
+
+/* Notify pinctrl_handler that igmp protocol column
+ * availability has changed. */
+notify_pinctrl_handler();
+}
+
ovs_mutex_unlock(_mutex);
}
@@ -5400,6 +5411,12 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
local_dp->datapath, chassis);
}
+/* Set Group protocol*/
+if (pinctrl.igmp_support_protocol) {
+igmp_group_set_protocol(sbrec_igmp,
+mc_group->protocol_version);
+}
+
igmp_group_update_ports(sbrec_igmp, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key, ip_ms->ms,
mc_group);
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index f3868068d..700c9cf6e 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -120,7 +120,7 @@ static const char *rbac_svc_monitor_auth_update[] =
static const char *rbac_igmp_group_auth[] =
{""};
static const char *rbac_igmp_group_update[] =
-{"address", "chassis", "datapath", "ports"};
+{"address", "protocol", "chassis", "datapath", "ports"};
static const char *rbac_bfd_auth[] =
{""};
static const char *rbac_bfd_update[] =
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 72e230b75..240d65f69 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Southbound",
-"version": "20.30.0",
-"cksum": "2972392849 31172",
+"version": "20.31.0",
+"cksum": "2643271413 31220",
"tables": {
"SB_Global": {
"columns": {
@@ -480,6 +480,7
[ovs-dev] [PATCH ovn 1/2] ovs: Bump submodule to include igmp protocol version.
Specifically the following commit:
077d0bad0436 ("mcast-snooping: Store IGMP/MLD protocol version.")
Also fix a small compilation error due to prototype change.
Signed-off-by: Mohammad Heib
---
controller/pinctrl.c | 6 +-
ovs | 2 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 4992eab08..77bf67e58 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -5474,9 +5474,13 @@ pinctrl_ip_mcast_handle_igmp(struct rconn *swconn,
switch (ntohs(ip_flow->tp_src)) {
case IGMP_HOST_MEMBERSHIP_REPORT:
case IGMPV2_HOST_MEMBERSHIP_REPORT:
+mcast_group_proto grp_proto =
+(ntohs(ip_flow->tp_src) == IGMP_HOST_MEMBERSHIP_REPORT)
+? MCAST_GROUP_IGMPV1
+: MCAST_GROUP_IGMPV2;
group_change =
mcast_snooping_add_group4(ip_ms->ms, ip4, IP_MCAST_VLAN,
- port_key_data);
+ port_key_data, grp_proto);
break;
case IGMP_HOST_LEAVE_MESSAGE:
group_change =
diff --git a/ovs b/ovs
index 4102674b3..b222593bc 16
--- a/ovs
+++ b/ovs
@@ -1 +1 @@
-Subproject commit 4102674b3ecadb0e20e512cc661cddbbc4b3d1f6
+Subproject commit b222593bc69b5d82849d18eb435564f5f93449d3
--
2.34.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests ovsdb-server: Fix config-file same schema test.
On Sat, Jan 20, 2024 at 09:42:52AM +, Frode Nordahl wrote:
> When a configuration file is used the ovsdb-server (re-)configures
> databases in multiple passes. First the configuration file is
> read and a shash is populated, second the shash is iterated over
> to remove/create databases.
>
> The "ovsdb-server config-file - same schema" test currently relies
> on a certain ordering of this shash, but we can't really rely on a
> specific ordering as it would be environment specific.
>
> The test currently fails on big endian systems such as s390x with:
> -WARN|failed to open database 'db2': ovsdb error: ordinals: duplicate
> database name
> +WARN|failed to open database 'db': ovsdb error: ordinals: duplicate database
> name
>
> Normalize the logged database name so that the test can focus on
> the fact that duplication is detected rather than in which order.
>
> Fixes: 55140090e63a ("ovsdb-server: Allow user-provided config files.")
> Signed-off-by: Frode Nordahl
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.
On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> With this change, a chassis may only update MAC Binding records that it
> has created. We achieve this by adding a "chassis_name" column to the
> MAC_Binding table, and having the chassis insert its name into this
> column when creating a new MAC_Binding. The "chassis_name" is now part
> of the rbac_auth structure for the MAC_Binding table.
Hi Mark,
i am concerned that this will negatively impact MAC_Bindings for LRPs
with multiple gateway chassis.
Suppose a MAC_Binding is first learned by an LRP currently residing on
chassis1. The LRP then failovers to chassis2 and chassis1 is potentially even
removed completely. In this case the ovn-controller on chassis2 would no
longer be allowed to update the timestamp column. This would break the
arp refresh mechanism.
In this case the MAC_Binding would need to expire first, causing northd
to removed it. Afterwards chassis2 would be allowed to insert a new
record with its own chassis name.
I honestly did not try out this case so i am not fully sure if this
issue realy exists or if i have a missunderstanding somewhere.
Thanks
Felix
> ---
> controller/pinctrl.c | 51
> northd/ovn-northd.c | 2 +-
> ovn-sb.ovsschema | 7 +++---
> ovn-sb.xml | 3 +++
> 4 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 4992eab08..a00cdceea 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -180,6 +180,7 @@ struct pinctrl {
> bool mac_binding_can_timestamp;
> bool fdb_can_timestamp;
> bool dns_supports_ovn_owned;
> +bool mac_binding_has_chassis_name;
> };
>
> static struct pinctrl pinctrl;
> @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> struct ovsdb_idl_index *sbrec_port_binding_by_key,
> -struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> +struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> +const struct sbrec_chassis *chassis)
> OVS_REQUIRES(pinctrl_mutex);
> static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
> static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> @@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl, const char
> *br_int_name)
> notify_pinctrl_handler();
> }
>
> +bool mac_binding_has_chassis_name =
> +sbrec_server_has_mac_binding_table_col_chassis_name(idl);
> +if (mac_binding_has_chassis_name !=
> pinctrl.mac_binding_has_chassis_name) {
> +pinctrl.mac_binding_has_chassis_name = mac_binding_has_chassis_name;
> +notify_pinctrl_handler();
> +}
> +
> ovs_mutex_unlock(_mutex);
> }
>
> @@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> ovs_mutex_lock(_mutex);
> run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key,
> - sbrec_mac_binding_by_lport_ip);
> + sbrec_mac_binding_by_lport_ip,
> + chassis);
> run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key, chassis);
> send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
> @@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>const char *logical_port,
>const struct sbrec_datapath_binding *dp,
>struct eth_addr ea, const char *ip,
> - bool update_only)
> + bool update_only,
> + const struct sbrec_chassis *chassis)
> {
> /* Convert ethernet argument to string form for database. */
> char mac_string[ETH_ADDR_STRLEN + 1];
> @@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> sbrec_mac_binding_set_logical_port(b, logical_port);
> sbrec_mac_binding_set_ip(b, ip);
> sbrec_mac_binding_set_datapath(b, dp);
> +if (pinctrl.mac_binding_has_chassis_name) {
> +sbrec_mac_binding_set_chassis_name(b, chassis->name);
> +}
> }
>
> if (strcmp(b->mac, mac_string)) {
> @@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
>struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
>const struct hmap *local_datapaths,
>const struct sbrec_port_binding *in_pb,
> - struct eth_addr ea, ovs_be32 ip)
> + struct eth_addr ea, ovs_be32 ip,
> + const struct sbrec_chassis *chassis)
> {
> if (!ovnsb_idl_txn) {
> return;
> @@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn
