| Message ID | 20241115234041.1323259-3-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | northd: Fix DHCPv6 flows that result in extra datapath matches. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maximets@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.maximets@ovn.org> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 11/19/24 09:06, Ales Musil wrote: > On Sat, Nov 16, 2024 at 12:41 AM Ilya Maximets <i.maximets@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.maximets@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? 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> >
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.maximets@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.maximets@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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Looks good to me, thanks. >> >> Acked-by: Ales Musil <amusil@redhat.com> >> >
On 11/21/24 00:28, Ilya Maximets wrote: > 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.maximets@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.maximets@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. > OK, I applied this one to main, 24.09 and 24.03. Regards, Dumitru > 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 >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >>> Looks good to me, thanks. >>> >>> Acked-by: Ales Musil <amusil@redhat.com> >>> >> >
On 11/21/24 10:27, Dumitru Ceara wrote: > On 11/21/24 00:28, Ilya Maximets wrote: >> 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.maximets@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.maximets@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. >> > > OK, I applied this one to main, 24.09 and 24.03. Could we also get this set backported to 23.09 as the previous similar IPv6 datapath flow fixes was? Thanks. Best regards, Ilya Maximets.
On 11/27/24 3:59 PM, Ilya Maximets wrote: > On 11/21/24 10:27, Dumitru Ceara wrote: >> On 11/21/24 00:28, Ilya Maximets wrote: >>> 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.maximets@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.maximets@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. >>> >> >> OK, I applied this one to main, 24.09 and 24.03. > > Could we also get this set backported to 23.09 as the previous > similar IPv6 datapath flow fixes was? > I backported this to 23.09 too now. However, the 23.09 branch will not generate any more stable releases, please see: https://mail.openvswitch.org/pipermail/ovs-announce/2024-October/000356.html We strongly recommend upgrading to 24.03 (LTS) or 24.09. Regards, Dumitru
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; };)
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.maximets@ovn.org> --- northd/northd.c | 9 ++++++--- northd/northd.h | 1 + tests/ovn-northd.at | 6 ------ 3 files changed, 7 insertions(+), 9 deletions(-)