[ovs-dev] [PATCH ovn v2 3/5] ovn-northd: Remove lflow_add_unique.

Han Zhou hzhou at ovn.org
Mon Jun 21 06:53:18 UTC 2021


On Fri, Jun 18, 2021 at 12:12 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 6/18/21 9:10 PM, Han Zhou wrote:
> > On Fri, Jun 18, 2021 at 11:57 AM Dumitru Ceara <dceara at redhat.com>
wrote:
> >>
> >> On 6/18/21 5:51 PM, Dumitru Ceara wrote:
> >>> On 6/11/21 9:35 PM, Han Zhou wrote:
> >>>> This patch removes the workaround when adding multicast group related
> >>>> lflows, because the multicast group dependency problem is fixed in
> >>>> ovn-controller in the previous commit.
> >>>>
> >>>> This patch also removes the UniqueFlow/AnnotatedFlow usage in northd
> >>>> DDlog implementation for the same reason.
> >>>>
> >>>> Signed-off-by: Han Zhou <hzhou at ovn.org>
> >>>> ---
> >>>>  northd/ovn-northd.c  |  89 ++++++-----------
> >>>>  northd/ovn_northd.dl | 233
+++++++++++++++++++------------------------
> >>>>  tests/ovn-northd.at  |   2 +-
> >>>>  3 files changed, 137 insertions(+), 187 deletions(-)
> >>>>
> >>>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> >>>> index 005c1fc86..411b14adf 100644
> >>>> --- a/northd/ovn-northd.c
> >>>> +++ b/northd/ovn-northd.c
> >>>> @@ -3663,9 +3663,6 @@ build_ports(struct northd_context *ctx,
> >>>>      sset_destroy(&active_ha_chassis_grps);
> >>>>  }
> >>>
> >>> [...]
> >>>
> >>>> @@ -6447,9 +6425,8 @@
> > build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op,
> >>>>
> >>>>      ds_put_format(&match, "eth.src == %s && (arp.op == 1 || nd_ns)",
> >>>>                    ds_cstr(&eth_src));
> >>>> -    ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
> >>>> -                         ds_cstr(&match),
> >>>> -                         "outport = \""MC_FLOOD_L2"\"; output;");
> >>>> +    ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
> > ds_cstr(&match),
> >>>> +                  "outport = \""MC_FLOOD_L2"\"; output;");
> >>>>
> >>>>      sset_destroy(&all_eth_addrs);
> >>>>      ds_destroy(&eth_src);
> >>>> @@ -6502,7 +6479,7 @@ build_lswitch_rport_arp_req_flow_for_ip(struct
> > sset *ips,
> >>>>          ds_put_format(&actions, "clone {outport = %s; output; }; "
> >>>>                                  "outport = \""MC_FLOOD_L2"\";
> > output;",
> >>>>                        patch_op->json_key);
> >>>> -        ovn_lflow_add_unique_with_hint(lflows, od,
> > S_SWITCH_IN_L2_LKUP,
> >>>> +        ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP,
> >>>>                                         priority, ds_cstr(&match),
> >>>>                                         ds_cstr(&actions),
stage_hint);
> >>>
> >>> Nit: indentation.
> >>>
> >>> Otherwise:
> >>>
> >>> Acked-by: Dumitru Ceara <dceara at redhat.com>
> >>>
> >>
> >> Actually, I'm not so sure about the ack anymore, with this applied some
> >> ovn-northd-ddlog tests fail, including:
> >>
> >> 772: ovn -- logical gatapath groups -- ovn-northd-ddlog -- dp-groups=no
> >> FAILED (ovn-macros.at:413)
> >>
> >> Regards,
> >> Dumitru
> >>
> > Thanks Dumitru for the review and test. All the test cases have passed
on
> > the earlier master branch, but after Numan's I-P patches for split
> > logical/physical flow_output I think I need to rebase this patch
series. I
> > didn't get time to do that yet, but I will do it in the next couple of
days
> > and re-test.
> >
>
> Looks like you need the following incremental for northd-ddlog.
>
> Regards,
> Dumitru
>
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 1323b935e..fb3840ac5 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1625,6 +1625,12 @@ UseLogicalDatapathGroups[false] :-
>      Unit(),
>      not nb in nb::NB_Global().
>
> +relation AnnotatedFlow(f: Flow, shared: bool)
> +AnnotatedFlow(f, b) :- UseLogicalDatapathGroups[b], Flow[f].
> +
> +relation UniqueFlow[Flow]
> +AnnotatedFlow(f, false) :- UniqueFlow[f].
> +
>  relation AggregatedFlow (
>      logical_datapaths: Set<uuid>,
>      stage:             Stage,
> @@ -1639,8 +1645,15 @@ AggregatedFlow(.logical_datapaths = g.to_set(),
>                 .__match = __match,
>                 .actions = actions,
>                 .external_ids = external_ids) :-
> -    Flow(logical_datapath, stage, priority, __match, actions,
external_ids),
> +    AnnotatedFlow(Flow{logical_datapath, stage, priority, __match,
actions, external_ids}, true),
>      var g = logical_datapath.group_by((stage, priority, __match,
actions, external_ids)).
> +AggregatedFlow(.logical_datapaths = set_singleton(logical_datapath),
> +               .stage = stage,
> +               .priority = priority,
> +               .__match = __match,
> +               .actions = actions,
> +               .external_ids = external_ids) :-
> +    AnnotatedFlow(Flow{logical_datapath, stage, priority, __match,
actions, external_ids}, false).
>
>  for (f in AggregatedFlow()) {
>      var pipeline = if (f.stage.pipeline == Ingress) "ingress" else
"egress" in
>

Thanks Dumitru. I see the problem. I must have missed rerunning the ddlog
test after this v2 patch and for whatever reason I believed that I ran it
successfully, really embarrassing.

So in v2 when removing the AnnotatedFlow I made a mistake and left
UseLogicalDatapathGroups unused, which caused this test failure. Your
suggestion works but it basically brought AnnotatedFlow back and reverted
to v1. I worked out a simpler version and sent v3:

https://patchwork.ozlabs.org/project/ovn/list/?series=249874

Thanks,
Han


More information about the dev mailing list