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

[Dumitru Ceara]

On 6/18/21 9:10 PM, Han Zhou wrote:
> On Fri, Jun 18, 2021 at 11:57 AM Dumitru Ceara <dce...@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 <hz...@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 <dce...@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

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev