On 4/3/25 9:54 AM, Ales Musil via dev wrote:
> On Mon, Mar 24, 2025 at 9:42 AM Aditya Mehakare <aditya.mehak...@nutanix.com>
> wrote:
>
>> This commit adds COPP support for DHCPv4 Relay.
>>
>> Acked-by: Naveen Yerramneni <naveen.yerramn...@nutanix.com>
>> Signed-off-by: Aditya Mehakare <aditya.mehak...@nutanix.com>
>> ---
>> v2: Handle intermittent test failure.
>> ---
>>
>
> Hi Aditya,
>
> thank you for the patch, I have some minor comments down below.
>
Hi Aditya, Ales,
> lib/copp.c | 1 +
>> lib/copp.h | 1 +
>> northd/northd.c | 20 +++++---
>> ovn-nb.xml | 4 ++
>> tests/ovn-northd.at | 2 +-
>> tests/ovn.at | 120 ++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 141 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/copp.c b/lib/copp.c
>> index 11dd9029d..8ba9ab7f4 100644
>> --- a/lib/copp.c
>> +++ b/lib/copp.c
>> @@ -40,6 +40,7 @@ static char *copp_proto_names[COPP_PROTO_MAX] = {
>> [COPP_REJECT] = "reject",
>> [COPP_SVC_MONITOR] = "svc-monitor",
>> [COPP_BFD] = "bfd",
>> + [COPP_DHCPV4_RELAY] = "dhcpv4-relay",
>> };
>>
>> static const char *
>> diff --git a/lib/copp.h b/lib/copp.h
>> index b99737220..3a7d1ccc5 100644
>> --- a/lib/copp.h
>> +++ b/lib/copp.h
>> @@ -38,6 +38,7 @@ enum copp_proto {
>> COPP_BFD,
>> COPP_REJECT,
>> COPP_SVC_MONITOR,
>> + COPP_DHCPV4_RELAY,
>> COPP_PROTO_MAX,
>> COPP_PROTO_INVALID = COPP_PROTO_MAX,
>> };
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 1d3e132d4..19cddd6c6 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -15098,7 +15098,8 @@ static void
>> build_dhcp_relay_flows_for_lrouter_port(struct ovn_port *op,
>> struct lflow_table *lflows,
>> struct ds *match, struct ds
>> *actions,
>> - struct lflow_ref *lflow_ref)
>> + struct lflow_ref *lflow_ref,
>> + const struct shash *meter_groups)
>> {
>> if (!op->nbrp || !op->nbrp->dhcp_relay ||
>> !op->lrp_networks.n_ipv4_addrs) {
>> return;
>> @@ -15147,8 +15148,11 @@ build_dhcp_relay_flows_for_lrouter_port(struct
>> ovn_port *op,
>> "next; /* DHCP_RELAY_REQ */",
>> op->lrp_networks.ipv4_addrs[0].addr_s, server_ip_str);
>>
>> - ovn_lflow_add_with_hint(lflows, op->od, S_ROUTER_IN_IP_INPUT, 110,
>> - ds_cstr(match), ds_cstr(actions),
>> + ovn_lflow_add_with_hint__(lflows, op->od, S_ROUTER_IN_IP_INPUT, 110,
>> + ds_cstr(match), ds_cstr(actions), NULL,
>> + copp_meter_get(COPP_DHCPV4_RELAY,
>> + op->od->nbr->copp,
>> + meter_groups),
>>
>
> nit: The arguments are not properly aligned.
>
> &op->nbrp->header_, lflow_ref);
>>
>> ds_clear(match);
>> @@ -15209,9 +15213,12 @@ build_dhcp_relay_flows_for_lrouter_port(struct
>> ovn_port *op,
>> " = dhcp_relay_resp_chk(%s, %s); next; /* DHCP_RELAY_RESP */",
>> op->lrp_networks.ipv4_addrs[0].addr_s, server_ip_str);
>>
>> - ovn_lflow_add_with_hint(lflows, op->od,
>> S_ROUTER_IN_DHCP_RELAY_RESP_CHK,
>> + ovn_lflow_add_with_hint__(lflows, op->od,
>> S_ROUTER_IN_DHCP_RELAY_RESP_CHK,
>> 100,
>> - ds_cstr(match), ds_cstr(actions),
>> + ds_cstr(match), ds_cstr(actions), NULL,
>> + copp_meter_get(COPP_DHCPV4_RELAY,
>> + op->od->nbr->copp,
>> + meter_groups),
>>
>
> nit: Same here.
>
> &op->nbrp->header_, lflow_ref);
>>
>>
>> @@ -17394,7 +17401,8 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct
>> ovn_port *op,
>> build_dhcpv6_reply_flows_for_lrouter_port(op, lsi->lflows,
>> &lsi->match,
>> op->lflow_ref);
>> build_dhcp_relay_flows_for_lrouter_port(op, lsi->lflows, &lsi->match,
>> - &lsi->actions, op->lflow_ref);
>> + &lsi->actions, op->lflow_ref,
>> + lsi->meter_groups);
>> build_ipv6_input_flows_for_lrouter_port(op, lsi->lflows,
>> &lsi->match, &lsi->actions,
>> lsi->meter_groups,
>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> index ff5f2f249..b72be9e59 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -640,6 +640,10 @@
>> Rate limiting meter for packets that are arriving to service
>> monitor MAC address.
>> </column>
>> + <column name="meters" key="dhcpv4-relay">
>> + Rate limiting meter for DHCPv4 relay packets (request/response) when
>> + DHCPv4 Relay functionality is enabled.
>> + </column>
>> <column name="external_ids">
>> See <em>External IDs</em> at the beginning of this document.
>> </column>
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 419130aa8..55605c443 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -4203,7 +4203,7 @@ AT_CHECK([ovn-sbctl list logical_flow | grep
>> trigger_event -A 2 | grep -q meter0
>>
>> # let's try to add an usupported protocol "dhcp"
>> AT_CHECK([ovn-nbctl --wait=hv copp-add copp5 dhcp meter1],[1],[],[dnl
>> -ovn-nbctl: Invalid control protocol. Allowed values: arp, arp-resolve,
>> dhcpv4-opts, dhcpv6-opts, dns, event-elb, icmp4-error, icmp6-error, igmp,
>> nd-na, nd-ns, nd-ns-resolve, nd-ra-opts, tcp-reset, bfd, reject,
>> svc-monitor.
>> +ovn-nbctl: Invalid control protocol. Allowed values: arp, arp-resolve,
>> dhcpv4-opts, dhcpv6-opts, dns, event-elb, icmp4-error, icmp6-error, igmp,
>> nd-na, nd-ns, nd-ns-resolve, nd-ra-opts, tcp-reset, bfd, reject,
>> svc-monitor, dhcpv4-relay.
>> ])
>>
>> #Let's try to add a valid protocol to an unknown datapath
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 714286596..235f324ed 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -40171,6 +40171,126 @@ n_packets=3
>>
>> sid=`ip_to_hex 172.16.1.1`
>>
>> +# ====================================================
>> +# Test Copp for DHCP request/response packets
>> +
>> +AT_CHECK([ovn-nbctl --wait=hv meter-add dhcp_relay_meter drop 10 pktps 5])
>> +AT_CHECK([ovn-nbctl --wait=hv copp-add copp_dhcp_relay dhcpv4-relay
>> dhcp_relay_meter])
>> +AT_CHECK([ovn-nbctl --wait=hv lr-copp-add copp_dhcp_relay lr0])
>>
>
> nit: All of those could be replaced with "check".
>
> +
>> +AT_CHECK([as hv1 ovs-appctl -t ovn-controller meter-table-list], [0], [dnl
>> +dhcp_relay_meter: 1
>> +])
>> +
>> +# Print some information that may help debugging.
>> +ovs-ofctl dump-flows br-int > pflows_dhcp_relay_copp
>> +ovn-sbctl find logical_flow controller_meter=dhcp_relay_meter >
>> lflows_dhcp_relay_copp
>> +
>> +## Verify packets within defined limit are not dropped.
>> +
>> +# Send DHCP discovery.
>> +src_mac="505400000010"
>> +src_ip=`ip_to_hex 0.0.0.0`
>> +dst_mac="ffffffffffff"
>> +dst_ip=`ip_to_hex 255.255.255.255`
>> +yiaddr=`ip_to_hex 0.0.0.0`
>> +giaddr=`ip_to_hex 0.0.0.0`
>> +sid=$src_ip
>>
>
> This is a great opportunity to use scapy for the send_dhcp_packet.
>
I agree we should. But let's not block this patch on it. Let's do that
as a follow up. Aditya, will you have time to do that by any chance?
> +
>> +for i in {1..3}
>> +do
>> + send_dhcp_packet $src_mac $src_ip $dst_mac $dst_ip 01 01 $yiaddr
>> $giaddr $sid vif0
>> +done
>> +
>> +echo "Meter stats:"
>> +as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int
>> +meter_stat=$(as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int)
>> +drop_cnt=$(echo $meter_stat | tail -n1 | sed -n
>> 's/.*packet_count:\([[0-9]]\+\).*/\1/p')
>> +AT_CHECK([ test $drop_cnt -eq 0 ], [0], [])
>>
>
> nit: Extra space around the test, also there is no need for the second and
> third argument.
>
> +
>> +# Send DHCP offer.
>> +src_mac="50540000001f"
>> +src_ip=`ip_to_hex 172.16.1.1`
>> +dst_mac="505400000002"
>> +dst_ip=`ip_to_hex 192.168.1.1`
>> +yiaddr=`ip_to_hex 192.168.1.10`
>> +giaddr=`ip_to_hex 192.168.1.1`
>> +sid=$src_ip
>> +
>> +sleep 1
>> +for i in {1..3}
>> +do
>> + send_dhcp_packet $src_mac $src_ip $dst_mac $dst_ip 02 02 $yiaddr
>> $giaddr $sid ext0
>> +done
>> +
>> +echo "Meter stats:"
>> +as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int
>> +meter_stat=$(as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int)
>> +drop_cnt=$(echo $meter_stat | tail -n1 | sed -n
>> 's/.*packet_count:\([[0-9]]\+\).*/\1/p')
>> +AT_CHECK([ test $drop_cnt -eq 0 ], [0], [])
>
>
> nit: Same here.
>
>
>> +
>> +## Verify packets exceeding rate limiting are dropped.
>> +
>> +# Send DHCP discovery.
>> +src_mac="505400000010"
>> +src_ip=`ip_to_hex 0.0.0.0`
>> +dst_mac="ffffffffffff"
>> +dst_ip=`ip_to_hex 255.255.255.255`
>> +yiaddr=`ip_to_hex 0.0.0.0`
>> +giaddr=`ip_to_hex 0.0.0.0`
>> +sid=$src_ip
>> +
>> +sleep 1
>> +start_time=$(date +%s%3N)
>> +for i in {1..15}
>> +do
>> + send_dhcp_packet $src_mac $src_ip $dst_mac $dst_ip 01 01 $yiaddr
>> $giaddr $sid vif0
>> +done
>> +end_time=$(date +%s%3N)
>> +
>> +# On particularly slow or overloaded systems, the transmission rate may
>> +# be lower than the configured meter rate. To prevent false test
>> +# failures, we check the transmission duration, and if it's
>> +# greater than expected time, just skip the test.
>> +d_ms=$(expr $end_time - $start_time)
>> +echo "Discover transmission duration ms: $d_ms"
>> +AT_SKIP_IF([test $d_ms -gt 1000])
>> +
>> +echo "Meter stats:"
>> +as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int
>> +meter_stat=$(as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int)
>> +drop_cnt=$(echo $meter_stat | tail -n1 | sed -n
>> 's/.*packet_count:\([[0-9]]\+\).*/\1/p')
>> +AT_CHECK([ test $drop_cnt -gt 1 ], [0], [])
>>
>
> nit: Same here.
>
>
>> +
>> +# Send DHCP offer.
>> +src_mac="50540000001f"
>> +src_ip=`ip_to_hex 172.16.1.1`
>> +dst_mac="505400000002"
>> +dst_ip=`ip_to_hex 192.168.1.1`
>> +yiaddr=`ip_to_hex 192.168.1.10`
>> +giaddr=`ip_to_hex 192.168.1.1`
>> +sid=$src_ip
>> +
>> +sleep 1
>> +start_time=$(date +%s%3N)
>> +for i in {1..15}
>> +do
>> + send_dhcp_packet $src_mac $src_ip $dst_mac $dst_ip 02 02 $yiaddr
>> $giaddr $sid ext0
>> +done
>> +end_time=$(date +%s%3N)
>> +
>> +d_ms=$(expr $end_time - $start_time)
>> +echo "Offer transmission duration ms: $d_ms"
>> +AT_SKIP_IF([test $d_ms -gt 1000])
>> +
>> +echo "Meter stats:"
>> +as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int
>> +prev_cnt=$drop_cnt
>> +meter_stat=$(as hv1 ovs-ofctl -O OpenFlow15 meter-stats br-int)
>> +drop_cnt=$(echo $meter_stat | tail -n1 | sed -n
>> 's/.*packet_count:\([[0-9]]\+\).*/\1/p')
>> +drop_delta=$(expr $drop_cnt - $prev_cnt)
>> +AT_CHECK([ test $drop_delta -gt 1 ], [0], [])
>>
>
> nit: Same here.
>
>
>
>> +
>> OVN_CLEANUP([hv1
>> /WARN\|DHCP_RELAY/d
>> ])
>> --
>> 2.43.5
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Thanks,
> Ales
I took care of the small nits, added a NEWS entry and pushed this patch
to main.
I also added Aditya to AUTHORS.rst.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
