On 11/19/24 23:42, Dumitru Ceara wrote:
> On 11/19/24 09:06, Ales Musil wrote:
>> On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>>> If the feature is not enabled, there is no need to create extra
>>> logical flows per network for each router port. These flows match on
>>> exact IPv6 addresses and UDP ports contributing to increased number of
>>> datapath flows generated in OVS on the nodes. This turns into exact
>>> matches in most cases potentially causing datapath flow explosion for
>>> the traffic entering OVN network from multiple sources.
>>>
>>> Flows removed from unrelated tests as a result.
>>>
>>> Fixes: 5c1d2d230773 ("northd: Add logical flows for dhcpv6 pfd parsing")
>>> Reported-at: https://issues.redhat.com/browse/FDP-992
>>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>>> ---
>
> Thanks, Ilya, for the fix. It makes sense. But as as soon as we enable
> the prefix delegation feature through config we'll end up in the same
> situation as before this change.
>
> I'm not against applying the patch, it's probably fine as is, but do we
> need to think of a follow up to fix the case when prefix delegation is
> enabled too?
Yeah, it makes sense to think about ways to avoid extra matches in this
case even if the prefix delegation is enabled. Though I'm not sure how
to implement that at the moment. Certainly something to think about as
a follow up.
Best regards, Ilya Maximets.
>
> Thanks,
> Dumitru
>
>>> northd/northd.c | 9 ++++++---
>>> northd/northd.h | 1 +
>>> tests/ovn-northd.at | 6 ------
>>> 3 files changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index 31fda8bb6..2aa6c0958 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -2330,6 +2330,9 @@ join_logical_ports(const struct
>>> sbrec_port_binding_table *sbrec_pb_table,
>>> op->lrp_networks = lrp_networks;
>>> op->od = od;
>>>
>>> + op->prefix_delegation = smap_get_bool(&op->nbrp->options,
>>> + "prefix_delegation",
>>> false);
>>> +
>>> for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) {
>>> sset_add(&op->od->router_ips,
>>> op->lrp_networks.ipv4_addrs[j].addr_s);
>>> @@ -7247,8 +7250,8 @@ ovn_update_ipv6_opt_for_op(struct ovn_port *op)
>>> smap_clone(&options, &op->sb->options);
>>>
>>> /* enable IPv6 prefix delegation */
>>> - bool prefix_delegation = smap_get_bool(&op->nbrp->options,
>>> - "prefix_delegation", false);
>>> + bool prefix_delegation = op->prefix_delegation;
>>> +
>>> if (!lrport_is_enabled(op->nbrp)) {
>>> prefix_delegation = false;
>>> }
>>> @@ -15081,7 +15084,7 @@ build_dhcpv6_reply_flows_for_lrouter_port(
>>> struct lflow_ref *lflow_ref)
>>> {
>>> ovs_assert(op->nbrp);
>>> - if (is_cr_port(op)) {
>>> + if (!op->prefix_delegation || is_cr_port(op)) {
>>> return;
>>> }
>>> for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
>>> diff --git a/northd/northd.h b/northd/northd.h
>>> index c1442ff40..e93e10f20 100644
>>> --- a/northd/northd.h
>>> +++ b/northd/northd.h
>>> @@ -620,6 +620,7 @@ struct ovn_port {
>>> const struct nbrec_logical_router_port *nbrp; /* May be NULL. */
>>>
>>> struct lport_addresses lrp_networks;
>>> + bool prefix_delegation; /* True if IPv6 prefix delegation enabled. */
>>>
>>> /* Logical port multicast data. */
>>> struct mcast_port_info mcast_info;
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index 8477e4250..e3b7b0cb5 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -13391,9 +13391,6 @@ AT_CHECK([grep "lr_in_ip_input" lr0flows |
>>> ovn_strip_lflows], [0], [dnl
>>> table=??(lr_in_ip_input ), priority=100 , match=(ip4.src ==
>>> {172.168.0.10, 172.168.0.255} && reg9[[0]] == 0), action=(drop;)
>>> table=??(lr_in_ip_input ), priority=100 , match=(ip4.src ==
>>> {20.0.0.1, 20.0.0.255} && reg9[[0]] == 0), action=(drop;)
>>> table=??(lr_in_ip_input ), priority=100 , match=(ip4.src_mcast
>>> ||ip4.src == 255.255.255.255 || ip4.src == 127.0.0.0/8 || ip4.dst ==
>>> 127.0.0.0/8 || ip4.src == 0.0.0.0/8 || ip4.dst == 0.0.0.0/8),
>>> action=(drop;)
>>> - table=??(lr_in_ip_input ), priority=100 , match=(ip6.dst ==
>>> fe80::200:ff:fe00:ff01 && udp.src == 547 && udp.dst == 546), action=(reg0 =
>>> 0; handle_dhcpv6_reply;)
>>> - table=??(lr_in_ip_input ), priority=100 , match=(ip6.dst ==
>>> fe80::200:ff:fe00:ff02 && udp.src == 547 && udp.dst == 546), action=(reg0 =
>>> 0; handle_dhcpv6_reply;)
>>> - table=??(lr_in_ip_input ), priority=100 , match=(ip6.dst ==
>>> fe80::200:ff:fe00:ff03 && udp.src == 547 && udp.dst == 546), action=(reg0 =
>>> 0; handle_dhcpv6_reply;)
>>> table=??(lr_in_ip_input ), priority=120 , match=(inport ==
>>> "lr0-public" && ip4.src == 172.168.0.100), action=(next;)
>>> table=??(lr_in_ip_input ), priority=30 , match=(ip.ttl == {0,
>>> 1}), action=(drop;)
>>> table=??(lr_in_ip_input ), priority=31 , match=(inport ==
>>> "lr0-public" && ip4 && ip.ttl == {0, 1} && !ip.later_frag), action=(icmp4
>>> {eth.dst <-> eth.src; icmp4.type = 11; /* Time exceeded */ icmp4.code = 0;
>>> /* TTL exceeded in transit */ ip4.dst <-> ip4.src ; ip.ttl = 254; outport =
>>> "lr0-public"; flags.loopback = 1; output; };)
>>> @@ -13570,9 +13567,6 @@ AT_CHECK([grep "lr_in_ip_input" lr0flows |
>>> ovn_strip_lflows], [0], [dnl
>>> table=??(lr_in_ip_input ), priority=100 , match=(ip4.src ==
>>> {172.168.0.10, 172.168.0.255} && reg9[[0]] == 0), action=(drop;)
>>> table=??(lr_in_ip_input ), priority=100 , match=(ip4.src ==
>>> {20.0.0.1, 20.0.0.255} && reg9[[0]] == 0), action=(drop;)
>>> table=??(lr_in_ip_input ), priority=100 , match=(ip4.src_mcast
>>> ||ip4.src == 255.255.255.255 || ip4.src == 127.0.0.0/8 || ip4.dst ==
>>> 127.0.0.0/8 || ip4.src == 0.0.0.0/8 || ip4.dst == 0.0.0.0/8),
>>> action=(drop;)
>>> - table=??(lr_in_ip_input ), priority=100 , match=(ip6.dst ==
>>> fe80::200:ff:fe00:ff01 && udp.src == 547 && udp.dst == 546), action=(reg0 =
>>> 0; handle_dhcpv6_reply;)
>>> - table=??(lr_in_ip_input ), priority=100 , match=(ip6.dst ==
>>> fe80::200:ff:fe00:ff02 && udp.src == 547 && udp.dst == 546), action=(reg0 =
>>> 0; handle_dhcpv6_reply;)
>>> - table=??(lr_in_ip_input ), priority=100 , match=(ip6.dst ==
>>> fe80::200:ff:fe00:ff03 && udp.src == 547 && udp.dst == 546), action=(reg0 =
>>> 0; handle_dhcpv6_reply;)
>>> table=??(lr_in_ip_input ), priority=120 , match=(inport ==
>>> "lr0-public" && ip4.src == 172.168.0.100), action=(next;)
>>> table=??(lr_in_ip_input ), priority=30 , match=(ip.ttl == {0,
>>> 1}), action=(drop;)
>>> table=??(lr_in_ip_input ), priority=31 , match=(inport ==
>>> "lr0-public" && ip4 && ip.ttl == {0, 1} && !ip.later_frag), action=(icmp4
>>> {eth.dst <-> eth.src; icmp4.type = 11; /* Time exceeded */ icmp4.code = 0;
>>> /* TTL exceeded in transit */ ip4.dst <-> ip4.src ; ip.ttl = 254; outport =
>>> "lr0-public"; flags.loopback = 1; output; };)
>>> --
>>> 2.47.0
>>>
>>> _______________________________________________
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>> Looks good to me, thanks.
>>
>> Acked-by: Ales Musil <amu...@redhat.com>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
