diff mbox series

[ovs-dev,2/4] ovn-northd: Remove lflow_add_unique.

Message ID 20210528192339.2707528-3-hzhou@ovn.org
State Superseded
Headers show
Series Fix ovn-controller I-P for multicast groups and lport changes | expand

Commit Message

Han Zhou May 28, 2021, 7:23 p.m. UTC
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@ovn.org>
---
 northd/ovn-northd.c  |  24 ++---
 northd/ovn_northd.dl | 220 +++++++++++++++++++++----------------------
 tests/ovn-northd.at  |   2 +-
 3 files changed, 118 insertions(+), 128 deletions(-)

Comments

Dumitru Ceara June 4, 2021, 1:56 p.m. UTC | #1
On 5/28/21 9:23 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@ovn.org>
> ---
>  northd/ovn-northd.c  |  24 ++---
>  northd/ovn_northd.dl | 220 +++++++++++++++++++++----------------------
>  tests/ovn-northd.at  |   2 +-
>  3 files changed, 118 insertions(+), 128 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index ca56a6efb..89d86596b 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c

There are some *unique*() leftovers that should probably be removed too:

- the comment above 'struct multicast_group {'
- ovn_lflow_add_unique_with_hint()
- ovn_lflow_add_unique()

ovn_lflow_add_at()/do_ovn_lflow_add() also need to be changed, and the
'shared' parameter can be removed.

> @@ -6443,7 +6443,7 @@ 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,
> +    ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
>                           ds_cstr(&match),
>                           "outport = \""MC_FLOOD_L2"\"; output;");

Indentation needs to be update too; this applies almost all occurrences
of ovn_lflow_add_unique().

>  
> @@ -6498,7 +6498,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);
>      } else {
> @@ -6854,7 +6854,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *lflows)
>                        "outport = get_fdb(eth.dst); next;");
>  
>          if (od->has_unknown) {
> -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
> +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
>                                   "outport == \"none\"",
>                                   "outport = \""MC_UNKNOWN "\"; output;");
>          } else {
> @@ -7300,24 +7300,24 @@ build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
>              }
>              ds_put_cstr(actions, "igmp;");
>              /* Punt IGMP traffic to controller. */
> -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
>                                   "ip4 && ip.proto == 2", ds_cstr(actions));
>  
>              /* Punt MLD traffic to controller. */
> -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
>                                   "mldv1 || mldv2", ds_cstr(actions));
>  
>              /* Flood all IP multicast traffic destined to 224.0.0.X to all
>               * ports - RFC 4541, section 2.1.2, item 2.
>               */
> -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
> +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
>                                   "ip4.mcast && ip4.dst == 224.0.0.0/24",
>                                   "outport = \""MC_FLOOD"\"; output;");
>  
>              /* Flood all IPv6 multicast traffic destined to reserved
>               * multicast IPs (RFC 4291, 2.7.1).
>               */
> -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
> +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
>                                   "ip6.mcast_flood",
>                                   "outport = \""MC_FLOOD"\"; output;");
>  
> @@ -7349,13 +7349,13 @@ build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
>                      ds_put_cstr(actions, "drop;");
>                  }
>  
> -                ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
> +                ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
>                                       "ip4.mcast || ip6.mcast",
>                                       ds_cstr(actions));
>              }
>          }
>  
> -        ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
> +        ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
>                               "outport = \""MC_FLOOD"\"; output;");
>      }
>  }
> @@ -7434,7 +7434,7 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group *igmp_group,
>          ds_put_format(actions, "outport = \"%s\"; output; ",
>                        igmp_group->mcgroup.name);
>  
> -        ovn_lflow_add_unique(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
> +        ovn_lflow_add(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
>                               90, ds_cstr(match), ds_cstr(actions));
>      }
>  }
> @@ -9976,7 +9976,7 @@ build_mcast_lookup_flows_for_lrouter(
>              }
>              ds_put_format(actions, "outport = \"%s\"; ip.ttl--; next;",
>                            igmp_group->mcgroup.name);
> -            ovn_lflow_add_unique(lflows, od, S_ROUTER_IN_IP_ROUTING, 500,
> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 500,
>                                   ds_cstr(match), ds_cstr(actions));
>          }
>  
> @@ -9984,7 +9984,7 @@ build_mcast_lookup_flows_for_lrouter(
>           * ports. Otherwise drop any multicast traffic.
>           */
>          if (od->mcast_info.rtr.flood_static) {
> -            ovn_lflow_add_unique(lflows, od, S_ROUTER_IN_IP_ROUTING, 450,
> +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 450,
>                            "ip4.mcast || ip6.mcast",
>                            "clone { "
>                                  "outport = \""MC_STATIC"\"; "
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index cb8418540..156eee43a 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1605,11 +1605,9 @@ function mFF_N_LOG_REGS()          : bit<32> = 10
>   *    - There's a setting "use_logical_dp_groups" that globally
>   *      enables or disables this feature.
>   *
> - *    - Some flows can't use this feature even if it's globally
> - *      enabled, due to ovn-controller bugs (see commit bfed224006750
> - *      "northd: Add support for Logical Datapath Groups.").  Flows
> - *      that can't be shared must get added into AnnotatedFlow with
> - *      'shared' set to 'false', instead of Flow.
> + *    - It is possible that some flows can't use this feature even if it's
> + *      globally enabled. Flows that can't be shared must get added into
> + *      AnnotatedFlow with 'shared' set to 'false', instead of Flow.

IIUC we don't need AnnotatedFlow anymore, as it was used for "unique flows".

>   */
>  
>  relation Flow(
> @@ -3812,42 +3810,42 @@ for (sw in &Switch(._uuid = ls_uuid, .mcast_cfg = mcast_cfg)
>                      }
>                  } in {
>                      /* Punt IGMP traffic to controller. */
> -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> -                                    .stage            = s_SWITCH_IN_L2_LKUP(),
> -                                    .priority         = 100,
> -                                    .__match          = "ip4 && ip.proto == 2",
> -                                    .actions          = "${igmp_act}",
> -                                    .external_ids     = map_empty()}];
> +                    Flow(.logical_datapath = ls_uuid,
> +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> +                         .priority         = 100,
> +                         .__match          = "ip4 && ip.proto == 2",
> +                         .actions          = "${igmp_act}",
> +                         .external_ids     = map_empty());
>  
>                      /* Punt MLD traffic to controller. */
> -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> -                                    .stage            = s_SWITCH_IN_L2_LKUP(),
> -                                    .priority         = 100,
> -                                    .__match          = "mldv1 || mldv2",
> -                                    .actions          = "${igmp_act}",
> -                                    .external_ids     = map_empty()}];
> +                    Flow(.logical_datapath = ls_uuid,
> +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> +                         .priority         = 100,
> +                         .__match          = "mldv1 || mldv2",
> +                         .actions          = "${igmp_act}",
> +                         .external_ids     = map_empty());
>  
>                      /* Flood all IP multicast traffic destined to 224.0.0.X to
>                       * all ports - RFC 4541, section 2.1.2, item 2.
>                       */
>                      var flood = json_string_escape(mC_FLOOD().0) in
> -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> -                                    .stage            = s_SWITCH_IN_L2_LKUP(),
> -                                    .priority         = 85,
> -                                    .__match          = "ip4.mcast && ip4.dst == 224.0.0.0/24",
> -                                    .actions          = "outport = ${flood}; output;",
> -                                    .external_ids     = map_empty()}];
> +                    Flow(.logical_datapath = ls_uuid,
> +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> +                         .priority         = 85,
> +                         .__match          = "ip4.mcast && ip4.dst == 224.0.0.0/24",
> +                         .actions          = "outport = ${flood}; output;",
> +                         .external_ids     = map_empty());
>  
>                      /* Flood all IPv6 multicast traffic destined to reserved
>                       * multicast IPs (RFC 4291, 2.7.1).
>                       */
>                      var flood = json_string_escape(mC_FLOOD().0) in
> -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> -                                    .stage            = s_SWITCH_IN_L2_LKUP(),
> -                                    .priority         = 85,
> -                                    .__match          = "ip6.mcast_flood",
> -                                    .actions          = "outport = ${flood}; output;",
> -                                    .external_ids     = map_empty()}];
> +                    Flow(.logical_datapath = ls_uuid,
> +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> +                         .priority         = 85,
> +                         .__match          = "ip6.mcast_flood",
> +                         .actions          = "outport = ${flood}; output;",
> +                         .external_ids     = map_empty());
>  
>                      /* Forward uregistered IP multicast to routers with relay
>                       * enabled and to any ports configured to flood IP
> @@ -3881,13 +3879,13 @@ for (sw in &Switch(._uuid = ls_uuid, .mcast_cfg = mcast_cfg)
>                                  ""
>                              }
>                          } in
> -                        UniqueFlow[Flow{.logical_datapath = ls_uuid,
> -                                        .stage            = s_SWITCH_IN_L2_LKUP(),
> -                                        .priority         = 80,
> -                                        .__match          = "ip4.mcast || ip6.mcast",
> -                                        .actions          =
> -                                           "${relay_act}${static_act}${drop_act}",
> -                                        .external_ids     = map_empty()}]
> +                        Flow(.logical_datapath = ls_uuid,
> +                             .stage            = s_SWITCH_IN_L2_LKUP(),
> +                             .priority         = 80,
> +                             .__match          = "ip4.mcast || ip6.mcast",
> +                             .actions          =
> +                                "${relay_act}${static_act}${drop_act}",
> +                                        .external_ids     = map_empty())
>                      }
>                  }
>              }
> @@ -3935,14 +3933,14 @@ for (IgmpSwitchMulticastGroup(.address = address, .switch = sw)) {
>                      ""
>                  }
>              } in
> -            UniqueFlow[Flow{.logical_datapath = sw._uuid,
> -                            .stage            = s_SWITCH_IN_L2_LKUP(),
> -                            .priority         = 90,
> -                            .__match          = "eth.mcast && ${ipX} && ${ipX}.dst == ${address}",
> -                            .actions          =
> -                               "${relay_act} ${static_act} outport = \"${address}\"; "
> -                               "output;",
> -                            .external_ids     = map_empty()}]
> +            Flow(.logical_datapath = sw._uuid,
> +                 .stage            = s_SWITCH_IN_L2_LKUP(),
> +                 .priority         = 90,
> +                 .__match          = "eth.mcast && ${ipX} && ${ipX}.dst == ${address}",
> +                 .actions          =
> +                    "${relay_act} ${static_act} outport = \"${address}\"; "
> +                    "output;",
> +                 .external_ids     = map_empty())
>          }
>      }
>  }
> @@ -4009,12 +4007,12 @@ Flow(.logical_datapath = sp.sw._uuid,
>   * (priority 100). */
>  for (ls in nb::Logical_Switch) {
>      var mc_flood = json_string_escape(mC_FLOOD().0) in
> -    UniqueFlow[Flow{.logical_datapath = ls._uuid,
> -                    .stage            = s_SWITCH_IN_L2_LKUP(),
> -                    .priority         = 70,
> -                    .__match          = "eth.mcast",
> -                    .actions          = "outport = ${mc_flood}; output;",
> -                    .external_ids     = map_empty()}]
> +    Flow(.logical_datapath = ls._uuid,
> +         .stage            = s_SWITCH_IN_L2_LKUP(),
> +         .priority         = 70,
> +         .__match          = "eth.mcast",
> +         .actions          = "outport = ${mc_flood}; output;",
> +         .external_ids     = map_empty())
>  }
>  
>  /* Ingress table L2_LKUP: Destination lookup, unicast handling (priority 50).
> @@ -4063,12 +4061,12 @@ function lrouter_port_ip_reachable(rp: Intern<RouterPort>, addr: v46_ip): bool {
>      };
>      false
>  }
> -UniqueFlow[Flow{.logical_datapath = sw._uuid,
> -                .stage            = s_SWITCH_IN_L2_LKUP(),
> -                .priority         = 75,
> -                .__match          = __match,
> -                .actions          = actions,
> -                .external_ids     = stage_hint(sp.lsp._uuid)}] :-
> +Flow(.logical_datapath = sw._uuid,
> +     .stage            = s_SWITCH_IN_L2_LKUP(),
> +     .priority         = 75,
> +     .__match          = __match,
> +     .actions          = actions,
> +     .external_ids     = stage_hint(sp.lsp._uuid)) :-
>      sp in &SwitchPort(.sw = sw@&Switch{.has_non_router_port = true}, .peer = Some{rp}),
>      rp.is_enabled(),
>      var eth_src_set = {
> @@ -4151,39 +4149,37 @@ function get_arp_forward_ips(rp: Intern<RouterPort>): (Set<string>, Set<string>)
>   * delivers to patch ports) but we're bypassing multicast_groups.
>   * (This is why we match against fLAGBIT_NOT_VXLAN() here.)
>   */
> -AnnotatedFlow(.f = Flow{.logical_datapath = sw._uuid,
> -                        .stage            = s_SWITCH_IN_L2_LKUP(),
> -                        .priority         = 80,
> -                        .__match          = fLAGBIT_NOT_VXLAN() ++
> -                                            " && arp.op == 1 && arp.tpa == { " ++
> -                                            all_ips_v4.to_vec().join(", ") ++ "}",
> -                        .actions          = if (sw.has_non_router_port) {
> -                                                "clone {outport = ${sp.json_name}; output; }; "
> -                                                "outport = ${mc_flood_l2}; output;"
> -                                            } else {
> -                                                "outport = ${sp.json_name}; output;"
> -                                            },
> -                        .external_ids     = stage_hint(sp.lsp._uuid)},
> -              .shared = not sw.has_non_router_port) :-
> +Flow(.logical_datapath = sw._uuid,
> +     .stage            = s_SWITCH_IN_L2_LKUP(),
> +     .priority         = 80,
> +     .__match          = fLAGBIT_NOT_VXLAN() ++
> +                         " && arp.op == 1 && arp.tpa == { " ++
> +                         all_ips_v4.to_vec().join(", ") ++ "}",
> +     .actions          = if (sw.has_non_router_port) {
> +                             "clone {outport = ${sp.json_name}; output; }; "
> +                             "outport = ${mc_flood_l2}; output;"
> +                         } else {
> +                             "outport = ${sp.json_name}; output;"
> +                         },
> +     .external_ids     = stage_hint(sp.lsp._uuid)) :-
>      sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
>      rp.is_enabled(),
>      (var all_ips_v4, _) = get_arp_forward_ips(rp),
>      not all_ips_v4.is_empty(),
>      var mc_flood_l2 = json_string_escape(mC_FLOOD_L2().0).
> -AnnotatedFlow(.f = Flow{.logical_datapath = sw._uuid,
> -                        .stage            = s_SWITCH_IN_L2_LKUP(),
> -                        .priority         = 80,
> -                        .__match          = fLAGBIT_NOT_VXLAN() ++
> -                                            " && nd_ns && nd.target == { " ++
> -                                            all_ips_v6.to_vec().join(", ") ++ "}",
> -                        .actions          = if (sw.has_non_router_port) {
> -                                                "clone {outport = ${sp.json_name}; output; }; "
> -                                                "outport = ${mc_flood_l2}; output;"
> -                                            } else {
> -                                                "outport = ${sp.json_name}; output;"
> -                                            },
> -                        .external_ids     = stage_hint(sp.lsp._uuid)},
> -              .shared = not sw.has_non_router_port) :-
> +Flow(.logical_datapath = sw._uuid,
> +     .stage            = s_SWITCH_IN_L2_LKUP(),
> +     .priority         = 80,
> +     .__match          = fLAGBIT_NOT_VXLAN() ++
> +                         " && nd_ns && nd.target == { " ++
> +                         all_ips_v6.to_vec().join(", ") ++ "}",
> +     .actions          = if (sw.has_non_router_port) {
> +                             "clone {outport = ${sp.json_name}; output; }; "
> +                             "outport = ${mc_flood_l2}; output;"
> +                         } else {
> +                             "outport = ${sp.json_name}; output;"
> +                         },
> +     .external_ids     = stage_hint(sp.lsp._uuid)) :-
>      sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
>      rp.is_enabled(),
>      (_, var all_ips_v6) = get_arp_forward_ips(rp),
> @@ -4279,22 +4275,17 @@ for (sw in &Switch(._uuid = ls_uuid)) {
>           .actions          = "outport = get_fdb(eth.dst); next;",
>           .external_ids     = map_empty());
>  
> -    if (sw.has_unknown_ports) {
> -        var mc_unknown = json_string_escape(mC_UNKNOWN().0) in
> -        UniqueFlow[Flow{.logical_datapath = ls_uuid,
> -                        .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> -                        .priority         = 50,
> -                        .__match          = "outport == \"none\"",
> -                        .actions          = "outport = ${mc_unknown}; output;",
> -                        .external_ids     = map_empty()}]
> -    } else {
> -        Flow(.logical_datapath = ls_uuid,
> -             .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> -             .priority         = 50,
> -             .__match          = "outport == \"none\"",
> -             .actions          = "drop;",
> -             .external_ids     = map_empty())
> -    };
> +    Flow(.logical_datapath = ls_uuid,
> +         .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> +         .priority         = 50,
> +         .__match          = "outport == \"none\"",
> +         .actions          = if (sw.has_unknown_ports) {
> +                                 var mc_unknown = json_string_escape(mC_UNKNOWN().0);
> +                                 "outport = ${mc_unknown}; output;"
> +                             } else {
> +                                 "drop;"
> +                             },
> +         .external_ids     = map_empty());
>  
>      Flow(.logical_datapath = ls_uuid,
>           .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> @@ -6638,14 +6629,14 @@ for (IgmpRouterMulticastGroup(address, rtr, ports)) {
>          } in
>          Some{var ip} = ip46_parse(address) in
>          var ipX = ip.ipX() in
> -        UniqueFlow[Flow{.logical_datapath = rtr._uuid,
> -                        .stage            = s_ROUTER_IN_IP_ROUTING(),
> -                        .priority         = 500,
> -                        .__match          = "${ipX} && ${ipX}.dst == ${address} ",
> -                        .actions          =
> -                           "${static_act}outport = ${json_string_escape(address)}; "
> -                           "ip.ttl--; next;",
> -                        .external_ids     = map_empty()}]
> +        Flow(.logical_datapath = rtr._uuid,
> +             .stage            = s_ROUTER_IN_IP_ROUTING(),
> +             .priority         = 500,
> +             .__match          = "${ipX} && ${ipX}.dst == ${address} ",
> +             .actions          =
> +                "${static_act}outport = ${json_string_escape(address)}; "
> +                "ip.ttl--; next;",
> +             .external_ids     = map_empty())]

There's an extra ']' here that breaks compilation.

>      }
>  }
>  
> @@ -6664,13 +6655,12 @@ for (RouterMcastFloodPorts(rtr, flood_ports) if rtr.mcast_cfg.relay) {
>      } else {
>          "drop;"
>      } in
> -    AnnotatedFlow(.f = Flow{.logical_datapath = rtr._uuid,
> -                            .stage            = s_ROUTER_IN_IP_ROUTING(),
> -                            .priority         = 450,
> -                            .__match          = "ip4.mcast || ip6.mcast",
> -                            .actions          = actions,
> -                            .external_ids     = map_empty()},
> -                  .shared = not flood_static)
> +    Flow(.logical_datapath = rtr._uuid,
> +         .stage            = s_ROUTER_IN_IP_ROUTING(),
> +         .priority         = 450,
> +         .__match          = "ip4.mcast || ip6.mcast",
> +         .actions          = actions,
> +         .external_ids     = map_empty())
>  }
>  
>  /* Logical router ingress table POLICY: Policy.
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 3c2aef4b0..dd20f9e7b 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2495,7 +2495,7 @@ check_row_count Logical_DP_Group 0
>  
>  dnl Number of logical flows that depends on logical switch or multicast group.
>  dnl These will not be combined.
> -n_flows_specific=$(ovn-sbctl --bare find Logical_Flow | grep -cE 'swp|_MC_')
> +n_flows_specific=$(ovn-sbctl --bare find Logical_Flow | grep -cE 'swp')
>  echo "Number of specific flows: "${n_flows_specific}
>  
>  dnl Both logical switches configured identically, so there should be same
> 

Thanks,
Dumitru
Han Zhou June 11, 2021, 7:36 p.m. UTC | #2
On Fri, Jun 4, 2021 at 6:56 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 5/28/21 9:23 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@ovn.org>
> > ---
> >  northd/ovn-northd.c  |  24 ++---
> >  northd/ovn_northd.dl | 220 +++++++++++++++++++++----------------------
> >  tests/ovn-northd.at  |   2 +-
> >  3 files changed, 118 insertions(+), 128 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index ca56a6efb..89d86596b 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
>
> There are some *unique*() leftovers that should probably be removed too:
>
> - the comment above 'struct multicast_group {'
> - ovn_lflow_add_unique_with_hint()
> - ovn_lflow_add_unique()
>
> ovn_lflow_add_at()/do_ovn_lflow_add() also need to be changed, and the
> 'shared' parameter can be removed.
>
Ack.

> > @@ -6443,7 +6443,7 @@ 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,
> > +    ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
> >                           ds_cstr(&match),
> >                           "outport = \""MC_FLOOD_L2"\"; output;");
>
> Indentation needs to be update too; this applies almost all occurrences
> of ovn_lflow_add_unique().
>
Ack.

> >
> > @@ -6498,7 +6498,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);
> >      } else {
> > @@ -6854,7 +6854,7 @@ build_lswitch_flows(struct hmap *datapaths,
struct hmap *lflows)
> >                        "outport = get_fdb(eth.dst); next;");
> >
> >          if (od->has_unknown) {
> > -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_UNKNOWN,
50,
> > +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
> >                                   "outport == \"none\"",
> >                                   "outport = \""MC_UNKNOWN "\";
output;");
> >          } else {
> > @@ -7300,24 +7300,24 @@ build_lswitch_destination_lookup_bmcast(struct
ovn_datapath *od,
> >              }
> >              ds_put_cstr(actions, "igmp;");
> >              /* Punt IGMP traffic to controller. */
> > -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> >                                   "ip4 && ip.proto == 2",
ds_cstr(actions));
> >
> >              /* Punt MLD traffic to controller. */
> > -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> >                                   "mldv1 || mldv2", ds_cstr(actions));
> >
> >              /* Flood all IP multicast traffic destined to 224.0.0.X to
all
> >               * ports - RFC 4541, section 2.1.2, item 2.
> >               */
> > -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
> > +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
> >                                   "ip4.mcast && ip4.dst == 224.0.0.0/24
",
> >                                   "outport = \""MC_FLOOD"\"; output;");
> >
> >              /* Flood all IPv6 multicast traffic destined to reserved
> >               * multicast IPs (RFC 4291, 2.7.1).
> >               */
> > -            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
> > +            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
> >                                   "ip6.mcast_flood",
> >                                   "outport = \""MC_FLOOD"\"; output;");
> >
> > @@ -7349,13 +7349,13 @@ build_lswitch_destination_lookup_bmcast(struct
ovn_datapath *od,
> >                      ds_put_cstr(actions, "drop;");
> >                  }
> >
> > -                ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP,
80,
> > +                ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
> >                                       "ip4.mcast || ip6.mcast",
> >                                       ds_cstr(actions));
> >              }
> >          }
> >
> > -        ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 70,
"eth.mcast",
> > +        ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
> >                               "outport = \""MC_FLOOD"\"; output;");
> >      }
> >  }
> > @@ -7434,7 +7434,7 @@ build_lswitch_ip_mcast_igmp_mld(struct
ovn_igmp_group *igmp_group,
> >          ds_put_format(actions, "outport = \"%s\"; output; ",
> >                        igmp_group->mcgroup.name);
> >
> > -        ovn_lflow_add_unique(lflows, igmp_group->datapath,
S_SWITCH_IN_L2_LKUP,
> > +        ovn_lflow_add(lflows, igmp_group->datapath,
S_SWITCH_IN_L2_LKUP,
> >                               90, ds_cstr(match), ds_cstr(actions));
> >      }
> >  }
> > @@ -9976,7 +9976,7 @@ build_mcast_lookup_flows_for_lrouter(
> >              }
> >              ds_put_format(actions, "outport = \"%s\"; ip.ttl--; next;",
> >                            igmp_group->mcgroup.name);
> > -            ovn_lflow_add_unique(lflows, od, S_ROUTER_IN_IP_ROUTING,
500,
> > +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 500,
> >                                   ds_cstr(match), ds_cstr(actions));
> >          }
> >
> > @@ -9984,7 +9984,7 @@ build_mcast_lookup_flows_for_lrouter(
> >           * ports. Otherwise drop any multicast traffic.
> >           */
> >          if (od->mcast_info.rtr.flood_static) {
> > -            ovn_lflow_add_unique(lflows, od, S_ROUTER_IN_IP_ROUTING,
450,
> > +            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 450,
> >                            "ip4.mcast || ip6.mcast",
> >                            "clone { "
> >                                  "outport = \""MC_STATIC"\"; "
> > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > index cb8418540..156eee43a 100644
> > --- a/northd/ovn_northd.dl
> > +++ b/northd/ovn_northd.dl
> > @@ -1605,11 +1605,9 @@ function mFF_N_LOG_REGS()          : bit<32> = 10
> >   *    - There's a setting "use_logical_dp_groups" that globally
> >   *      enables or disables this feature.
> >   *
> > - *    - Some flows can't use this feature even if it's globally
> > - *      enabled, due to ovn-controller bugs (see commit bfed224006750
> > - *      "northd: Add support for Logical Datapath Groups.").  Flows
> > - *      that can't be shared must get added into AnnotatedFlow with
> > - *      'shared' set to 'false', instead of Flow.
> > + *    - It is possible that some flows can't use this feature even if
it's
> > + *      globally enabled. Flows that can't be shared must get added
into
> > + *      AnnotatedFlow with 'shared' set to 'false', instead of Flow.
>
> IIUC we don't need AnnotatedFlow anymore, as it was used for "unique
flows".
>
I kept it in case it is useful for other future use cases, but after
revisiting it I agree it is better to completely remove it for now.

> >   */
> >
> >  relation Flow(
> > @@ -3812,42 +3810,42 @@ for (sw in &Switch(._uuid = ls_uuid, .mcast_cfg
= mcast_cfg)
> >                      }
> >                  } in {
> >                      /* Punt IGMP traffic to controller. */
> > -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> > -                                    .stage            =
s_SWITCH_IN_L2_LKUP(),
> > -                                    .priority         = 100,
> > -                                    .__match          = "ip4 &&
ip.proto == 2",
> > -                                    .actions          = "${igmp_act}",
> > -                                    .external_ids     = map_empty()}];
> > +                    Flow(.logical_datapath = ls_uuid,
> > +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> > +                         .priority         = 100,
> > +                         .__match          = "ip4 && ip.proto == 2",
> > +                         .actions          = "${igmp_act}",
> > +                         .external_ids     = map_empty());
> >
> >                      /* Punt MLD traffic to controller. */
> > -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> > -                                    .stage            =
s_SWITCH_IN_L2_LKUP(),
> > -                                    .priority         = 100,
> > -                                    .__match          = "mldv1 ||
mldv2",
> > -                                    .actions          = "${igmp_act}",
> > -                                    .external_ids     = map_empty()}];
> > +                    Flow(.logical_datapath = ls_uuid,
> > +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> > +                         .priority         = 100,
> > +                         .__match          = "mldv1 || mldv2",
> > +                         .actions          = "${igmp_act}",
> > +                         .external_ids     = map_empty());
> >
> >                      /* Flood all IP multicast traffic destined to
224.0.0.X to
> >                       * all ports - RFC 4541, section 2.1.2, item 2.
> >                       */
> >                      var flood = json_string_escape(mC_FLOOD().0) in
> > -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> > -                                    .stage            =
s_SWITCH_IN_L2_LKUP(),
> > -                                    .priority         = 85,
> > -                                    .__match          = "ip4.mcast &&
ip4.dst == 224.0.0.0/24",
> > -                                    .actions          = "outport =
${flood}; output;",
> > -                                    .external_ids     = map_empty()}];
> > +                    Flow(.logical_datapath = ls_uuid,
> > +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> > +                         .priority         = 85,
> > +                         .__match          = "ip4.mcast && ip4.dst ==
224.0.0.0/24",
> > +                         .actions          = "outport = ${flood};
output;",
> > +                         .external_ids     = map_empty());
> >
> >                      /* Flood all IPv6 multicast traffic destined to
reserved
> >                       * multicast IPs (RFC 4291, 2.7.1).
> >                       */
> >                      var flood = json_string_escape(mC_FLOOD().0) in
> > -                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
> > -                                    .stage            =
s_SWITCH_IN_L2_LKUP(),
> > -                                    .priority         = 85,
> > -                                    .__match          =
"ip6.mcast_flood",
> > -                                    .actions          = "outport =
${flood}; output;",
> > -                                    .external_ids     = map_empty()}];
> > +                    Flow(.logical_datapath = ls_uuid,
> > +                         .stage            = s_SWITCH_IN_L2_LKUP(),
> > +                         .priority         = 85,
> > +                         .__match          = "ip6.mcast_flood",
> > +                         .actions          = "outport = ${flood};
output;",
> > +                         .external_ids     = map_empty());
> >
> >                      /* Forward uregistered IP multicast to routers
with relay
> >                       * enabled and to any ports configured to flood IP
> > @@ -3881,13 +3879,13 @@ for (sw in &Switch(._uuid = ls_uuid, .mcast_cfg
= mcast_cfg)
> >                                  ""
> >                              }
> >                          } in
> > -                        UniqueFlow[Flow{.logical_datapath = ls_uuid,
> > -                                        .stage            =
s_SWITCH_IN_L2_LKUP(),
> > -                                        .priority         = 80,
> > -                                        .__match          = "ip4.mcast
|| ip6.mcast",
> > -                                        .actions          =
> > -
"${relay_act}${static_act}${drop_act}",
> > -                                        .external_ids     =
map_empty()}]
> > +                        Flow(.logical_datapath = ls_uuid,
> > +                             .stage            = s_SWITCH_IN_L2_LKUP(),
> > +                             .priority         = 80,
> > +                             .__match          = "ip4.mcast ||
ip6.mcast",
> > +                             .actions          =
> > +                                "${relay_act}${static_act}${drop_act}",
> > +                                        .external_ids     =
map_empty())
> >                      }
> >                  }
> >              }
> > @@ -3935,14 +3933,14 @@ for (IgmpSwitchMulticastGroup(.address =
address, .switch = sw)) {
> >                      ""
> >                  }
> >              } in
> > -            UniqueFlow[Flow{.logical_datapath = sw._uuid,
> > -                            .stage            = s_SWITCH_IN_L2_LKUP(),
> > -                            .priority         = 90,
> > -                            .__match          = "eth.mcast && ${ipX}
&& ${ipX}.dst == ${address}",
> > -                            .actions          =
> > -                               "${relay_act} ${static_act} outport =
\"${address}\"; "
> > -                               "output;",
> > -                            .external_ids     = map_empty()}]
> > +            Flow(.logical_datapath = sw._uuid,
> > +                 .stage            = s_SWITCH_IN_L2_LKUP(),
> > +                 .priority         = 90,
> > +                 .__match          = "eth.mcast && ${ipX} &&
${ipX}.dst == ${address}",
> > +                 .actions          =
> > +                    "${relay_act} ${static_act} outport =
\"${address}\"; "
> > +                    "output;",
> > +                 .external_ids     = map_empty())
> >          }
> >      }
> >  }
> > @@ -4009,12 +4007,12 @@ Flow(.logical_datapath = sp.sw._uuid,
> >   * (priority 100). */
> >  for (ls in nb::Logical_Switch) {
> >      var mc_flood = json_string_escape(mC_FLOOD().0) in
> > -    UniqueFlow[Flow{.logical_datapath = ls._uuid,
> > -                    .stage            = s_SWITCH_IN_L2_LKUP(),
> > -                    .priority         = 70,
> > -                    .__match          = "eth.mcast",
> > -                    .actions          = "outport = ${mc_flood};
output;",
> > -                    .external_ids     = map_empty()}]
> > +    Flow(.logical_datapath = ls._uuid,
> > +         .stage            = s_SWITCH_IN_L2_LKUP(),
> > +         .priority         = 70,
> > +         .__match          = "eth.mcast",
> > +         .actions          = "outport = ${mc_flood}; output;",
> > +         .external_ids     = map_empty())
> >  }
> >
> >  /* Ingress table L2_LKUP: Destination lookup, unicast handling
(priority 50).
> > @@ -4063,12 +4061,12 @@ function lrouter_port_ip_reachable(rp:
Intern<RouterPort>, addr: v46_ip): bool {
> >      };
> >      false
> >  }
> > -UniqueFlow[Flow{.logical_datapath = sw._uuid,
> > -                .stage            = s_SWITCH_IN_L2_LKUP(),
> > -                .priority         = 75,
> > -                .__match          = __match,
> > -                .actions          = actions,
> > -                .external_ids     = stage_hint(sp.lsp._uuid)}] :-
> > +Flow(.logical_datapath = sw._uuid,
> > +     .stage            = s_SWITCH_IN_L2_LKUP(),
> > +     .priority         = 75,
> > +     .__match          = __match,
> > +     .actions          = actions,
> > +     .external_ids     = stage_hint(sp.lsp._uuid)) :-
> >      sp in &SwitchPort(.sw = sw@&Switch{.has_non_router_port = true},
.peer = Some{rp}),
> >      rp.is_enabled(),
> >      var eth_src_set = {
> > @@ -4151,39 +4149,37 @@ function get_arp_forward_ips(rp:
Intern<RouterPort>): (Set<string>, Set<string>)
> >   * delivers to patch ports) but we're bypassing multicast_groups.
> >   * (This is why we match against fLAGBIT_NOT_VXLAN() here.)
> >   */
> > -AnnotatedFlow(.f = Flow{.logical_datapath = sw._uuid,
> > -                        .stage            = s_SWITCH_IN_L2_LKUP(),
> > -                        .priority         = 80,
> > -                        .__match          = fLAGBIT_NOT_VXLAN() ++
> > -                                            " && arp.op == 1 &&
arp.tpa == { " ++
> > -
 all_ips_v4.to_vec().join(", ") ++ "}",
> > -                        .actions          = if
(sw.has_non_router_port) {
> > -                                                "clone {outport =
${sp.json_name}; output; }; "
> > -                                                "outport =
${mc_flood_l2}; output;"
> > -                                            } else {
> > -                                                "outport =
${sp.json_name}; output;"
> > -                                            },
> > -                        .external_ids     = stage_hint(sp.lsp._uuid)},
> > -              .shared = not sw.has_non_router_port) :-
> > +Flow(.logical_datapath = sw._uuid,
> > +     .stage            = s_SWITCH_IN_L2_LKUP(),
> > +     .priority         = 80,
> > +     .__match          = fLAGBIT_NOT_VXLAN() ++
> > +                         " && arp.op == 1 && arp.tpa == { " ++
> > +                         all_ips_v4.to_vec().join(", ") ++ "}",
> > +     .actions          = if (sw.has_non_router_port) {
> > +                             "clone {outport = ${sp.json_name};
output; }; "
> > +                             "outport = ${mc_flood_l2}; output;"
> > +                         } else {
> > +                             "outport = ${sp.json_name}; output;"
> > +                         },
> > +     .external_ids     = stage_hint(sp.lsp._uuid)) :-
> >      sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> >      rp.is_enabled(),
> >      (var all_ips_v4, _) = get_arp_forward_ips(rp),
> >      not all_ips_v4.is_empty(),
> >      var mc_flood_l2 = json_string_escape(mC_FLOOD_L2().0).
> > -AnnotatedFlow(.f = Flow{.logical_datapath = sw._uuid,
> > -                        .stage            = s_SWITCH_IN_L2_LKUP(),
> > -                        .priority         = 80,
> > -                        .__match          = fLAGBIT_NOT_VXLAN() ++
> > -                                            " && nd_ns && nd.target ==
{ " ++
> > -
 all_ips_v6.to_vec().join(", ") ++ "}",
> > -                        .actions          = if
(sw.has_non_router_port) {
> > -                                                "clone {outport =
${sp.json_name}; output; }; "
> > -                                                "outport =
${mc_flood_l2}; output;"
> > -                                            } else {
> > -                                                "outport =
${sp.json_name}; output;"
> > -                                            },
> > -                        .external_ids     = stage_hint(sp.lsp._uuid)},
> > -              .shared = not sw.has_non_router_port) :-
> > +Flow(.logical_datapath = sw._uuid,
> > +     .stage            = s_SWITCH_IN_L2_LKUP(),
> > +     .priority         = 80,
> > +     .__match          = fLAGBIT_NOT_VXLAN() ++
> > +                         " && nd_ns && nd.target == { " ++
> > +                         all_ips_v6.to_vec().join(", ") ++ "}",
> > +     .actions          = if (sw.has_non_router_port) {
> > +                             "clone {outport = ${sp.json_name};
output; }; "
> > +                             "outport = ${mc_flood_l2}; output;"
> > +                         } else {
> > +                             "outport = ${sp.json_name}; output;"
> > +                         },
> > +     .external_ids     = stage_hint(sp.lsp._uuid)) :-
> >      sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
> >      rp.is_enabled(),
> >      (_, var all_ips_v6) = get_arp_forward_ips(rp),
> > @@ -4279,22 +4275,17 @@ for (sw in &Switch(._uuid = ls_uuid)) {
> >           .actions          = "outport = get_fdb(eth.dst); next;",
> >           .external_ids     = map_empty());
> >
> > -    if (sw.has_unknown_ports) {
> > -        var mc_unknown = json_string_escape(mC_UNKNOWN().0) in
> > -        UniqueFlow[Flow{.logical_datapath = ls_uuid,
> > -                        .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> > -                        .priority         = 50,
> > -                        .__match          = "outport == \"none\"",
> > -                        .actions          = "outport = ${mc_unknown};
output;",
> > -                        .external_ids     = map_empty()}]
> > -    } else {
> > -        Flow(.logical_datapath = ls_uuid,
> > -             .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> > -             .priority         = 50,
> > -             .__match          = "outport == \"none\"",
> > -             .actions          = "drop;",
> > -             .external_ids     = map_empty())
> > -    };
> > +    Flow(.logical_datapath = ls_uuid,
> > +         .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> > +         .priority         = 50,
> > +         .__match          = "outport == \"none\"",
> > +         .actions          = if (sw.has_unknown_ports) {
> > +                                 var mc_unknown =
json_string_escape(mC_UNKNOWN().0);
> > +                                 "outport = ${mc_unknown}; output;"
> > +                             } else {
> > +                                 "drop;"
> > +                             },
> > +         .external_ids     = map_empty());
> >
> >      Flow(.logical_datapath = ls_uuid,
> >           .stage            = s_SWITCH_IN_L2_UNKNOWN(),
> > @@ -6638,14 +6629,14 @@ for (IgmpRouterMulticastGroup(address, rtr,
ports)) {
> >          } in
> >          Some{var ip} = ip46_parse(address) in
> >          var ipX = ip.ipX() in
> > -        UniqueFlow[Flow{.logical_datapath = rtr._uuid,
> > -                        .stage            = s_ROUTER_IN_IP_ROUTING(),
> > -                        .priority         = 500,
> > -                        .__match          = "${ipX} && ${ipX}.dst ==
${address} ",
> > -                        .actions          =
> > -                           "${static_act}outport =
${json_string_escape(address)}; "
> > -                           "ip.ttl--; next;",
> > -                        .external_ids     = map_empty()}]
> > +        Flow(.logical_datapath = rtr._uuid,
> > +             .stage            = s_ROUTER_IN_IP_ROUTING(),
> > +             .priority         = 500,
> > +             .__match          = "${ipX} && ${ipX}.dst == ${address} ",
> > +             .actions          =
> > +                "${static_act}outport =
${json_string_escape(address)}; "
> > +                "ip.ttl--; next;",
> > +             .external_ids     = map_empty())]
>
> There's an extra ']' here that breaks compilation.

Oops, really sorry about that.

Thanks,
Han

>
> >      }
> >  }
> >
> > @@ -6664,13 +6655,12 @@ for (RouterMcastFloodPorts(rtr, flood_ports) if
rtr.mcast_cfg.relay) {
> >      } else {
> >          "drop;"
> >      } in
> > -    AnnotatedFlow(.f = Flow{.logical_datapath = rtr._uuid,
> > -                            .stage            =
s_ROUTER_IN_IP_ROUTING(),
> > -                            .priority         = 450,
> > -                            .__match          = "ip4.mcast ||
ip6.mcast",
> > -                            .actions          = actions,
> > -                            .external_ids     = map_empty()},
> > -                  .shared = not flood_static)
> > +    Flow(.logical_datapath = rtr._uuid,
> > +         .stage            = s_ROUTER_IN_IP_ROUTING(),
> > +         .priority         = 450,
> > +         .__match          = "ip4.mcast || ip6.mcast",
> > +         .actions          = actions,
> > +         .external_ids     = map_empty())
> >  }
> >
> >  /* Logical router ingress table POLICY: Policy.
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 3c2aef4b0..dd20f9e7b 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2495,7 +2495,7 @@ check_row_count Logical_DP_Group 0
> >
> >  dnl Number of logical flows that depends on logical switch or
multicast group.
> >  dnl These will not be combined.
> > -n_flows_specific=$(ovn-sbctl --bare find Logical_Flow | grep -cE
'swp|_MC_')
> > +n_flows_specific=$(ovn-sbctl --bare find Logical_Flow | grep -cE 'swp')
> >  echo "Number of specific flows: "${n_flows_specific}
> >
> >  dnl Both logical switches configured identically, so there should be
same
> >
>
> Thanks,
> Dumitru
>
diff mbox series

Patch

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index ca56a6efb..89d86596b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -6443,7 +6443,7 @@  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,
+    ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
                          ds_cstr(&match),
                          "outport = \""MC_FLOOD_L2"\"; output;");
 
@@ -6498,7 +6498,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);
     } else {
@@ -6854,7 +6854,7 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *lflows)
                       "outport = get_fdb(eth.dst); next;");
 
         if (od->has_unknown) {
-            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
+            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
                                  "outport == \"none\"",
                                  "outport = \""MC_UNKNOWN "\"; output;");
         } else {
@@ -7300,24 +7300,24 @@  build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
             }
             ds_put_cstr(actions, "igmp;");
             /* Punt IGMP traffic to controller. */
-            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
+            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
                                  "ip4 && ip.proto == 2", ds_cstr(actions));
 
             /* Punt MLD traffic to controller. */
-            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
+            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
                                  "mldv1 || mldv2", ds_cstr(actions));
 
             /* Flood all IP multicast traffic destined to 224.0.0.X to all
              * ports - RFC 4541, section 2.1.2, item 2.
              */
-            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
+            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
                                  "ip4.mcast && ip4.dst == 224.0.0.0/24",
                                  "outport = \""MC_FLOOD"\"; output;");
 
             /* Flood all IPv6 multicast traffic destined to reserved
              * multicast IPs (RFC 4291, 2.7.1).
              */
-            ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
+            ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
                                  "ip6.mcast_flood",
                                  "outport = \""MC_FLOOD"\"; output;");
 
@@ -7349,13 +7349,13 @@  build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
                     ds_put_cstr(actions, "drop;");
                 }
 
-                ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
+                ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
                                      "ip4.mcast || ip6.mcast",
                                      ds_cstr(actions));
             }
         }
 
-        ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
+        ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
                              "outport = \""MC_FLOOD"\"; output;");
     }
 }
@@ -7434,7 +7434,7 @@  build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group *igmp_group,
         ds_put_format(actions, "outport = \"%s\"; output; ",
                       igmp_group->mcgroup.name);
 
-        ovn_lflow_add_unique(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
+        ovn_lflow_add(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
                              90, ds_cstr(match), ds_cstr(actions));
     }
 }
@@ -9976,7 +9976,7 @@  build_mcast_lookup_flows_for_lrouter(
             }
             ds_put_format(actions, "outport = \"%s\"; ip.ttl--; next;",
                           igmp_group->mcgroup.name);
-            ovn_lflow_add_unique(lflows, od, S_ROUTER_IN_IP_ROUTING, 500,
+            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 500,
                                  ds_cstr(match), ds_cstr(actions));
         }
 
@@ -9984,7 +9984,7 @@  build_mcast_lookup_flows_for_lrouter(
          * ports. Otherwise drop any multicast traffic.
          */
         if (od->mcast_info.rtr.flood_static) {
-            ovn_lflow_add_unique(lflows, od, S_ROUTER_IN_IP_ROUTING, 450,
+            ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 450,
                           "ip4.mcast || ip6.mcast",
                           "clone { "
                                 "outport = \""MC_STATIC"\"; "
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index cb8418540..156eee43a 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -1605,11 +1605,9 @@  function mFF_N_LOG_REGS()          : bit<32> = 10
  *    - There's a setting "use_logical_dp_groups" that globally
  *      enables or disables this feature.
  *
- *    - Some flows can't use this feature even if it's globally
- *      enabled, due to ovn-controller bugs (see commit bfed224006750
- *      "northd: Add support for Logical Datapath Groups.").  Flows
- *      that can't be shared must get added into AnnotatedFlow with
- *      'shared' set to 'false', instead of Flow.
+ *    - It is possible that some flows can't use this feature even if it's
+ *      globally enabled. Flows that can't be shared must get added into
+ *      AnnotatedFlow with 'shared' set to 'false', instead of Flow.
  */
 
 relation Flow(
@@ -3812,42 +3810,42 @@  for (sw in &Switch(._uuid = ls_uuid, .mcast_cfg = mcast_cfg)
                     }
                 } in {
                     /* Punt IGMP traffic to controller. */
-                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
-                                    .stage            = s_SWITCH_IN_L2_LKUP(),
-                                    .priority         = 100,
-                                    .__match          = "ip4 && ip.proto == 2",
-                                    .actions          = "${igmp_act}",
-                                    .external_ids     = map_empty()}];
+                    Flow(.logical_datapath = ls_uuid,
+                         .stage            = s_SWITCH_IN_L2_LKUP(),
+                         .priority         = 100,
+                         .__match          = "ip4 && ip.proto == 2",
+                         .actions          = "${igmp_act}",
+                         .external_ids     = map_empty());
 
                     /* Punt MLD traffic to controller. */
-                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
-                                    .stage            = s_SWITCH_IN_L2_LKUP(),
-                                    .priority         = 100,
-                                    .__match          = "mldv1 || mldv2",
-                                    .actions          = "${igmp_act}",
-                                    .external_ids     = map_empty()}];
+                    Flow(.logical_datapath = ls_uuid,
+                         .stage            = s_SWITCH_IN_L2_LKUP(),
+                         .priority         = 100,
+                         .__match          = "mldv1 || mldv2",
+                         .actions          = "${igmp_act}",
+                         .external_ids     = map_empty());
 
                     /* Flood all IP multicast traffic destined to 224.0.0.X to
                      * all ports - RFC 4541, section 2.1.2, item 2.
                      */
                     var flood = json_string_escape(mC_FLOOD().0) in
-                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
-                                    .stage            = s_SWITCH_IN_L2_LKUP(),
-                                    .priority         = 85,
-                                    .__match          = "ip4.mcast && ip4.dst == 224.0.0.0/24",
-                                    .actions          = "outport = ${flood}; output;",
-                                    .external_ids     = map_empty()}];
+                    Flow(.logical_datapath = ls_uuid,
+                         .stage            = s_SWITCH_IN_L2_LKUP(),
+                         .priority         = 85,
+                         .__match          = "ip4.mcast && ip4.dst == 224.0.0.0/24",
+                         .actions          = "outport = ${flood}; output;",
+                         .external_ids     = map_empty());
 
                     /* Flood all IPv6 multicast traffic destined to reserved
                      * multicast IPs (RFC 4291, 2.7.1).
                      */
                     var flood = json_string_escape(mC_FLOOD().0) in
-                    UniqueFlow[Flow{.logical_datapath = ls_uuid,
-                                    .stage            = s_SWITCH_IN_L2_LKUP(),
-                                    .priority         = 85,
-                                    .__match          = "ip6.mcast_flood",
-                                    .actions          = "outport = ${flood}; output;",
-                                    .external_ids     = map_empty()}];
+                    Flow(.logical_datapath = ls_uuid,
+                         .stage            = s_SWITCH_IN_L2_LKUP(),
+                         .priority         = 85,
+                         .__match          = "ip6.mcast_flood",
+                         .actions          = "outport = ${flood}; output;",
+                         .external_ids     = map_empty());
 
                     /* Forward uregistered IP multicast to routers with relay
                      * enabled and to any ports configured to flood IP
@@ -3881,13 +3879,13 @@  for (sw in &Switch(._uuid = ls_uuid, .mcast_cfg = mcast_cfg)
                                 ""
                             }
                         } in
-                        UniqueFlow[Flow{.logical_datapath = ls_uuid,
-                                        .stage            = s_SWITCH_IN_L2_LKUP(),
-                                        .priority         = 80,
-                                        .__match          = "ip4.mcast || ip6.mcast",
-                                        .actions          =
-                                           "${relay_act}${static_act}${drop_act}",
-                                        .external_ids     = map_empty()}]
+                        Flow(.logical_datapath = ls_uuid,
+                             .stage            = s_SWITCH_IN_L2_LKUP(),
+                             .priority         = 80,
+                             .__match          = "ip4.mcast || ip6.mcast",
+                             .actions          =
+                                "${relay_act}${static_act}${drop_act}",
+                                        .external_ids     = map_empty())
                     }
                 }
             }
@@ -3935,14 +3933,14 @@  for (IgmpSwitchMulticastGroup(.address = address, .switch = sw)) {
                     ""
                 }
             } in
-            UniqueFlow[Flow{.logical_datapath = sw._uuid,
-                            .stage            = s_SWITCH_IN_L2_LKUP(),
-                            .priority         = 90,
-                            .__match          = "eth.mcast && ${ipX} && ${ipX}.dst == ${address}",
-                            .actions          =
-                               "${relay_act} ${static_act} outport = \"${address}\"; "
-                               "output;",
-                            .external_ids     = map_empty()}]
+            Flow(.logical_datapath = sw._uuid,
+                 .stage            = s_SWITCH_IN_L2_LKUP(),
+                 .priority         = 90,
+                 .__match          = "eth.mcast && ${ipX} && ${ipX}.dst == ${address}",
+                 .actions          =
+                    "${relay_act} ${static_act} outport = \"${address}\"; "
+                    "output;",
+                 .external_ids     = map_empty())
         }
     }
 }
@@ -4009,12 +4007,12 @@  Flow(.logical_datapath = sp.sw._uuid,
  * (priority 100). */
 for (ls in nb::Logical_Switch) {
     var mc_flood = json_string_escape(mC_FLOOD().0) in
-    UniqueFlow[Flow{.logical_datapath = ls._uuid,
-                    .stage            = s_SWITCH_IN_L2_LKUP(),
-                    .priority         = 70,
-                    .__match          = "eth.mcast",
-                    .actions          = "outport = ${mc_flood}; output;",
-                    .external_ids     = map_empty()}]
+    Flow(.logical_datapath = ls._uuid,
+         .stage            = s_SWITCH_IN_L2_LKUP(),
+         .priority         = 70,
+         .__match          = "eth.mcast",
+         .actions          = "outport = ${mc_flood}; output;",
+         .external_ids     = map_empty())
 }
 
 /* Ingress table L2_LKUP: Destination lookup, unicast handling (priority 50).
@@ -4063,12 +4061,12 @@  function lrouter_port_ip_reachable(rp: Intern<RouterPort>, addr: v46_ip): bool {
     };
     false
 }
-UniqueFlow[Flow{.logical_datapath = sw._uuid,
-                .stage            = s_SWITCH_IN_L2_LKUP(),
-                .priority         = 75,
-                .__match          = __match,
-                .actions          = actions,
-                .external_ids     = stage_hint(sp.lsp._uuid)}] :-
+Flow(.logical_datapath = sw._uuid,
+     .stage            = s_SWITCH_IN_L2_LKUP(),
+     .priority         = 75,
+     .__match          = __match,
+     .actions          = actions,
+     .external_ids     = stage_hint(sp.lsp._uuid)) :-
     sp in &SwitchPort(.sw = sw@&Switch{.has_non_router_port = true}, .peer = Some{rp}),
     rp.is_enabled(),
     var eth_src_set = {
@@ -4151,39 +4149,37 @@  function get_arp_forward_ips(rp: Intern<RouterPort>): (Set<string>, Set<string>)
  * delivers to patch ports) but we're bypassing multicast_groups.
  * (This is why we match against fLAGBIT_NOT_VXLAN() here.)
  */
-AnnotatedFlow(.f = Flow{.logical_datapath = sw._uuid,
-                        .stage            = s_SWITCH_IN_L2_LKUP(),
-                        .priority         = 80,
-                        .__match          = fLAGBIT_NOT_VXLAN() ++
-                                            " && arp.op == 1 && arp.tpa == { " ++
-                                            all_ips_v4.to_vec().join(", ") ++ "}",
-                        .actions          = if (sw.has_non_router_port) {
-                                                "clone {outport = ${sp.json_name}; output; }; "
-                                                "outport = ${mc_flood_l2}; output;"
-                                            } else {
-                                                "outport = ${sp.json_name}; output;"
-                                            },
-                        .external_ids     = stage_hint(sp.lsp._uuid)},
-              .shared = not sw.has_non_router_port) :-
+Flow(.logical_datapath = sw._uuid,
+     .stage            = s_SWITCH_IN_L2_LKUP(),
+     .priority         = 80,
+     .__match          = fLAGBIT_NOT_VXLAN() ++
+                         " && arp.op == 1 && arp.tpa == { " ++
+                         all_ips_v4.to_vec().join(", ") ++ "}",
+     .actions          = if (sw.has_non_router_port) {
+                             "clone {outport = ${sp.json_name}; output; }; "
+                             "outport = ${mc_flood_l2}; output;"
+                         } else {
+                             "outport = ${sp.json_name}; output;"
+                         },
+     .external_ids     = stage_hint(sp.lsp._uuid)) :-
     sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
     rp.is_enabled(),
     (var all_ips_v4, _) = get_arp_forward_ips(rp),
     not all_ips_v4.is_empty(),
     var mc_flood_l2 = json_string_escape(mC_FLOOD_L2().0).
-AnnotatedFlow(.f = Flow{.logical_datapath = sw._uuid,
-                        .stage            = s_SWITCH_IN_L2_LKUP(),
-                        .priority         = 80,
-                        .__match          = fLAGBIT_NOT_VXLAN() ++
-                                            " && nd_ns && nd.target == { " ++
-                                            all_ips_v6.to_vec().join(", ") ++ "}",
-                        .actions          = if (sw.has_non_router_port) {
-                                                "clone {outport = ${sp.json_name}; output; }; "
-                                                "outport = ${mc_flood_l2}; output;"
-                                            } else {
-                                                "outport = ${sp.json_name}; output;"
-                                            },
-                        .external_ids     = stage_hint(sp.lsp._uuid)},
-              .shared = not sw.has_non_router_port) :-
+Flow(.logical_datapath = sw._uuid,
+     .stage            = s_SWITCH_IN_L2_LKUP(),
+     .priority         = 80,
+     .__match          = fLAGBIT_NOT_VXLAN() ++
+                         " && nd_ns && nd.target == { " ++
+                         all_ips_v6.to_vec().join(", ") ++ "}",
+     .actions          = if (sw.has_non_router_port) {
+                             "clone {outport = ${sp.json_name}; output; }; "
+                             "outport = ${mc_flood_l2}; output;"
+                         } else {
+                             "outport = ${sp.json_name}; output;"
+                         },
+     .external_ids     = stage_hint(sp.lsp._uuid)) :-
     sp in &SwitchPort(.sw = sw, .peer = Some{rp}),
     rp.is_enabled(),
     (_, var all_ips_v6) = get_arp_forward_ips(rp),
@@ -4279,22 +4275,17 @@  for (sw in &Switch(._uuid = ls_uuid)) {
          .actions          = "outport = get_fdb(eth.dst); next;",
          .external_ids     = map_empty());
 
-    if (sw.has_unknown_ports) {
-        var mc_unknown = json_string_escape(mC_UNKNOWN().0) in
-        UniqueFlow[Flow{.logical_datapath = ls_uuid,
-                        .stage            = s_SWITCH_IN_L2_UNKNOWN(),
-                        .priority         = 50,
-                        .__match          = "outport == \"none\"",
-                        .actions          = "outport = ${mc_unknown}; output;",
-                        .external_ids     = map_empty()}]
-    } else {
-        Flow(.logical_datapath = ls_uuid,
-             .stage            = s_SWITCH_IN_L2_UNKNOWN(),
-             .priority         = 50,
-             .__match          = "outport == \"none\"",
-             .actions          = "drop;",
-             .external_ids     = map_empty())
-    };
+    Flow(.logical_datapath = ls_uuid,
+         .stage            = s_SWITCH_IN_L2_UNKNOWN(),
+         .priority         = 50,
+         .__match          = "outport == \"none\"",
+         .actions          = if (sw.has_unknown_ports) {
+                                 var mc_unknown = json_string_escape(mC_UNKNOWN().0);
+                                 "outport = ${mc_unknown}; output;"
+                             } else {
+                                 "drop;"
+                             },
+         .external_ids     = map_empty());
 
     Flow(.logical_datapath = ls_uuid,
          .stage            = s_SWITCH_IN_L2_UNKNOWN(),
@@ -6638,14 +6629,14 @@  for (IgmpRouterMulticastGroup(address, rtr, ports)) {
         } in
         Some{var ip} = ip46_parse(address) in
         var ipX = ip.ipX() in
-        UniqueFlow[Flow{.logical_datapath = rtr._uuid,
-                        .stage            = s_ROUTER_IN_IP_ROUTING(),
-                        .priority         = 500,
-                        .__match          = "${ipX} && ${ipX}.dst == ${address} ",
-                        .actions          =
-                           "${static_act}outport = ${json_string_escape(address)}; "
-                           "ip.ttl--; next;",
-                        .external_ids     = map_empty()}]
+        Flow(.logical_datapath = rtr._uuid,
+             .stage            = s_ROUTER_IN_IP_ROUTING(),
+             .priority         = 500,
+             .__match          = "${ipX} && ${ipX}.dst == ${address} ",
+             .actions          =
+                "${static_act}outport = ${json_string_escape(address)}; "
+                "ip.ttl--; next;",
+             .external_ids     = map_empty())]
     }
 }
 
@@ -6664,13 +6655,12 @@  for (RouterMcastFloodPorts(rtr, flood_ports) if rtr.mcast_cfg.relay) {
     } else {
         "drop;"
     } in
-    AnnotatedFlow(.f = Flow{.logical_datapath = rtr._uuid,
-                            .stage            = s_ROUTER_IN_IP_ROUTING(),
-                            .priority         = 450,
-                            .__match          = "ip4.mcast || ip6.mcast",
-                            .actions          = actions,
-                            .external_ids     = map_empty()},
-                  .shared = not flood_static)
+    Flow(.logical_datapath = rtr._uuid,
+         .stage            = s_ROUTER_IN_IP_ROUTING(),
+         .priority         = 450,
+         .__match          = "ip4.mcast || ip6.mcast",
+         .actions          = actions,
+         .external_ids     = map_empty())
 }
 
 /* Logical router ingress table POLICY: Policy.
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 3c2aef4b0..dd20f9e7b 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2495,7 +2495,7 @@  check_row_count Logical_DP_Group 0
 
 dnl Number of logical flows that depends on logical switch or multicast group.
 dnl These will not be combined.
-n_flows_specific=$(ovn-sbctl --bare find Logical_Flow | grep -cE 'swp|_MC_')
+n_flows_specific=$(ovn-sbctl --bare find Logical_Flow | grep -cE 'swp')
 echo "Number of specific flows: "${n_flows_specific}
 
 dnl Both logical switches configured identically, so there should be same