On 11/15/23 07:45, Han Zhou wrote: > On Thu, Oct 26, 2023 at 11:16 AM <[email protected]> wrote: >> >> From: Numan Siddique <[email protected]> >> >> This is not required. >> > Thanks Numan for the fix. Could you provide a little more detail why this > is not required: > - Is it a bug fix? What's the impact? > - Shall we update the ovn-northd documentation for the related lflow > changes?
I agree with Han, the commit log could be improved. I think the ovn-northd documentation was already behind wrt this flow. There was no explicit mention of it anywhere. I'm pretty sure it's safe to not commit dhcp responses in conntrack, we bypass conntrack for the requests anyway: https://github.com/ovn-org/ovn/blob/5ef2a08ade01d698f84e197987ea679d8978d2b9/northd/northd.c#L7215-L7224 > - Is there a reason this is in the I-P patch series? Guessing again, I'm still reviewing the rest of the series, but probably to avoid an unnecessary dependency on logical switch data (if switches have stateful ACLs applied). Regards, Dumitru > > Thanks, > Han > >> Signed-off-by: Numan Siddique <[email protected]> >> --- >> northd/northd.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 1877cbc7df..c8a224d3cd 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -9223,9 +9223,7 @@ build_dhcpv4_options_flows(struct ovn_port *op, >> &op->nbsp->dhcpv4_options->options, "lease_time"); >> ovs_assert(server_id && server_mac && lease_time); >> const char *dhcp_actions = >> - (op->od->has_stateful_acl || op->od->has_lb_vip) >> - ? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit; next;" >> - : REGBIT_ACL_VERDICT_ALLOW" = 1; next;"; >> + REGBIT_ACL_VERDICT_ALLOW" = 1; next;"; >> ds_clear(&match); >> ds_put_format(&match, "outport == %s && eth.src == %s " >> "&& ip4.src == %s && udp && udp.src == 67 " >> @@ -9308,9 +9306,7 @@ build_dhcpv6_options_flows(struct ovn_port *op, >> ipv6_string_mapped(server_ip, &lla); >> >> const char *dhcp6_actions = >> - (op->od->has_stateful_acl || op->od->has_lb_vip) >> - ? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit; > next;" >> - : REGBIT_ACL_VERDICT_ALLOW" = 1; next;"; >> + REGBIT_ACL_VERDICT_ALLOW" = 1; next;"; >> ds_clear(&match); >> ds_put_format(&match, "outport == %s && eth.src == %s " >> "&& ip6.src == %s && udp && udp.src == 547 > " >> -- >> 2.41.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
