On Fri, Jun 4, 2021 at 6:56 AM Dumitru Ceara <dce...@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 <hz...@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(ð_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
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
