| Message ID | 20231026181539.3367113-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | northd lflow incremental processing | 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 | fail | github build: failed |
On Thu, Oct 26, 2023 at 11:16 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > 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? - Is there a reason this is in the I-P patch series? Thanks, Han > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 11/15/23 07:45, Han Zhou wrote: > On Thu, Oct 26, 2023 at 11:16 AM <numans@ovn.org> wrote: >> >> From: Numan Siddique <numans@ovn.org> >> >> 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 <numans@ovn.org> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Nov 23, 2023 at 4:26 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 11/15/23 07:45, Han Zhou wrote: > > On Thu, Oct 26, 2023 at 11:16 AM <numans@ovn.org> wrote: > >> > >> From: Numan Siddique <numans@ovn.org> > >> > >> 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). Correct. I've squashed this commit with the next one in v3. so that there is no confusion. And updated the commit message accordingly. Numan > > Regards, > Dumitru > > > > > Thanks, > > Han > > > >> Signed-off-by: Numan Siddique <numans@ovn.org> > >> --- > >> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 "