[ovs-dev] [PATCH v2 ovn] northd: reduce number of nd_na lb logical flows

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Thu Sep 9 12:42:54 UTC 2021


> On 8/27/21 2:00 PM, Lorenzo Bianconi wrote:
> > > Hi Lorenzo,
> > > 
> > > This mostly looks good, but I have one question below:
> > > 
> > > On 8/17/21 1:51 PM, Lorenzo Bianconi wrote:
> > > > As it has been already done for IPv4, collapse IPv6 Neighbour
> > > > Advertisment flows for load balancer using address list and
> > > > reduce the number of logical flows from N*M to N where N is
> > > > the number of logical router port and M is the number of
> > > > Virtual IPs.
> > > > 
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1970258
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > > > ---
> > > > Changes since v1:
> > > > - rebase on top of ovn master
> > > > - add DDlog support
> > > > ---
> > > >    northd/ovn-northd.c  | 41 +++++++++++++++++++--------
> > > >    northd/ovn_northd.dl | 66 ++++++++++++++++++++++++++++++++------------
> > > >    tests/ovn-northd.at  | 36 ++++++++----------------
> > > >    3 files changed, 90 insertions(+), 53 deletions(-)
> > > > 
> > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > index e80876af1..ee08281c3 100644
> > > > --- a/northd/ovn-northd.c
> > > > +++ b/northd/ovn-northd.c
> > > > @@ -9694,8 +9694,7 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct ovn_port *op,
> > > >            ds_put_format(&actions,
> > > >                          "%s { "
> > > >                            "eth.src = %s; "
> > > > -                        "ip6.src = %s; "
> > > > -                        "nd.target = %s; "
> > > > +                        "ip6.src = nd.target; "
> > > >                            "nd.tll = %s; "
> > > >                            "outport = inport; "
> > > >                            "flags.loopback = 1; "
> > > > @@ -9703,8 +9702,6 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct ovn_port *op,
> > > >                          "};",
> > > >                          action,
> > > >                          eth_addr,
> > > > -                      ip_address,
> > > > -                      ip_address,
> > > >                          eth_addr);
> > > >            ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_IP_INPUT, priority,
> > > >                                      ds_cstr(&match), ds_cstr(&actions), NULL,
> > > > @@ -11742,7 +11739,6 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
> > > >                                       &op->nbrp->header_, lflows);
> > > >            }
> > > > -        const char *ip_address;
> > > >            if (sset_count(&op->od->lb_ips_v4)) {
> > > >                ds_clear(match);
> > > >                if (is_l3dgw_port(op)) {
> > > > @@ -11763,17 +11759,40 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
> > > >                ds_destroy(&load_balancer_ips_v4);
> > > >            }
> > > > -        SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
> > > > +        if (sset_count(&op->od->lb_ips_v6)) {
> > > >                ds_clear(match);
> > > > +            ds_clear(actions);
> > > > +
> > > > +            struct ds load_balancer_ips_v6 = DS_EMPTY_INITIALIZER;
> > > > +
> > > > +            ds_put_cstr(&load_balancer_ips_v6, "{ ");
> > > > +            ds_put_and_free_cstr(&load_balancer_ips_v6,
> > > > +                                 sset_join(&op->od->lb_ips_v6, ", ", " }"));
> > > > +
> > > > +            ds_put_format(match, "inport == %s && nd_ns && nd.target == %s",
> > > > +                          op->json_key, ds_cstr(&load_balancer_ips_v6));
> > > >                if (is_l3dgw_port(op)) {
> > > > -                ds_put_format(match, "is_chassis_resident(%s)",
> > > > +                ds_put_format(match, " && is_chassis_resident(%s)",
> > > >                                  op->cr_port->json_key);
> > > >                }
> > > > +            ds_put_format(actions,
> > > > +                          "nd_na { "
> > > > +                            "eth.src = %s; "
> > > > +                            "ip6.src = nd.target; "
> > > > +                            "nd.tll = %s; "
> > > > +                            "outport = inport; "
> > > > +                            "flags.loopback = 1; "
> > > > +                            "output; "
> > > > +                          "};",
> > > > +                          REG_INPORT_ETH_ADDR,
> > > > +                          REG_INPORT_ETH_ADDR);
> > > > +            ovn_lflow_add_with_hint__(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
> > > > +                                      ds_cstr(match), ds_cstr(actions), NULL,
> > > > +                                      copp_meter_get(COPP_ND_NA,
> > > > +                                                     op->od->nbr->copp,
> > > > +                                                     meter_groups), NULL);
> > > 
> > > I don't understand why you replaced the call to build_lrouter_nd_flow() with
> > > manual creation of the match and actions. As long as you pass
> > > ds_cstr(&load_balancer_ips_v6) as the "ip_address" parameter, I think
> > > build_lrouter_nd_flow() will generate the proper flow.
> > 
> > Hi Mark,
> > 
> > IIRC it is because build_lrouter_nd_flow() is run even in other places where it is
> > not correct to have  match = ... nd.target == {addr0, addr1} and action = ..
> > ip6.src = nd.target since addr1 can be a multicast address.
> > 
> > Regards,
> > Lorenzo
> 
> Hi Lorenzo, sorry I'm not following your reasoning here. My thought here was
> that with your change, you generate a logical flow like:

ack Mark, you are right. I will address your comments in v3.

Regrads,
Lorenzo

> 
> match: inport == <port> && nd_ns && nd_target == {IP1, IP2, ...} [&&
> is_chassis_resident(<cr_port>)]
> action: nd_na {
>     eth.src = REG_INPORT_ETH_ADDR;
>     ip6.src = nd.target;
>     nd.tll = REG_INPORT_ETH_ADDR;
>     outport = inport;
>     flags.loopback = 1;
>     output;
> };
> 
> If you instead called
> 
> build_router_nd_flow(op->od, op, "nd_na", ds_cstr(&load_balancer_ips_v6),
> NULL, REG_INPORT_ETH_ADDR, match, false, 90, NULL, lflows, meter_groups)
> 
> You end up with the exact same match and actions.
> 
> I'm not sure what bearing this has on other places in the code calling
> build_router_nd_flow()
> 
> > 
> > > 
> > > > -            build_lrouter_nd_flow(op->od, op, "nd_na",
> > > > -                                  ip_address, NULL, REG_INPORT_ETH_ADDR,
> > > > -                                  match, false, 90, NULL,
> > > > -                                  lflows, meter_groups);
> > > > +            ds_destroy(&load_balancer_ips_v6);
> > > >            }
> > > >            if (!op->od->is_gw_router && !op->od->n_l3dgw_ports) {
> > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> > > > index 6cc5d7248..947637f2a 100644
> > > > --- a/northd/ovn_northd.dl
> > > > +++ b/northd/ovn_northd.dl
> > > > @@ -5260,6 +5260,44 @@ LogicalRouterNdFlow(router, lrp, "nd_na", ipv6, true, mac, extra_match, drop, pr
> > > >        LogicalRouterArpNdFlow(router, nat at NAT{.external_ip = IPv6{ipv6}}, lrp,
> > > >                               mac, extra_match, drop, priority).
> > > > +relation LogicalRouterNdFlowLB(
> > > > +    lr: Intern<Router>,
> > > > +    lrp: Option<Intern<nb::Logical_Router_Port>>,
> > > > +    ip: string,
> > > > +    mac: string,
> > > > +    extra_match: Option<string>,
> > > > +    external_ids: Map<string,string>)
> > > > +MeteredFlow(.logical_datapath = lr._uuid,
> > > > +            .stage = s_ROUTER_IN_IP_INPUT(),
> > > > +            .priority = 90,
> > > > +            .__match = __match,
> > > > +            .actions = actions,
> > > > +            .tags             = map_empty(),
> > > > +            .controller_meter = lr.copp.get(cOPP_ND_NA()),
> > > > +            .external_ids = external_ids) :-
> > > > +    LogicalRouterNdFlowLB(.lr = lr, .lrp = lrp, .ip = ip,
> > > > +                          .mac = mac, .extra_match = extra_match,
> > > > +                          .external_ids = external_ids),
> > > > +    var __match = {
> > > > +        var clauses = vec_with_capacity(4);
> > > > +        match (lrp) {
> > > > +            Some{p} -> clauses.push("inport == ${json_string_escape(p.name)}"),
> > > > +            None -> ()
> > > > +        };
> > > > +        clauses.push("nd_ns && nd.target == ${ip}");
> > > > +        clauses.append(extra_match.to_vec());
> > > > +        clauses.join(" && ")
> > > > +    },
> > > > +    var actions =
> > > > +        "nd_na { "
> > > > +           "eth.src = ${mac}; "
> > > > +           "ip6.src = nd.target; "
> > > > +           "nd.tll = ${mac}; "
> > > > +           "outport = inport; "
> > > > +           "flags.loopback = 1; "
> > > > +           "output; "
> > > > +         "};".
> > > > +
> > > >    relation LogicalRouterArpFlow(
> > > >        lr: Intern<Router>,
> > > >        lrp: Option<Intern<nb::Logical_Router_Port>>,
> > > > @@ -5343,8 +5381,7 @@ MeteredFlow(.logical_datapath = lr._uuid,
> > > >        } else {
> > > >            ("${action} { "
> > > >               "eth.src = ${mac}; "
> > > > -           "ip6.src = ${ip}; "
> > > > -           "nd.target = ${ip}; "
> > > > +           "ip6.src = nd.target; "
> > > >               "nd.tll = ${mac}; "
> > > >               "outport = inport; "
> > > >               "flags.loopback = 1; "
> > > > @@ -5412,7 +5449,7 @@ var residence_check = match (is_redirect) {
> > > >        true -> Some{"is_chassis_resident(${json_string_escape(chassis_redirect_name(lrp.name))})"},
> > > >        false -> None
> > > >    } in {
> > > > -    (var all_ips_v4, _) = get_router_load_balancer_ips(router, false) in {
> > > > +    (var all_ips_v4, var all_ips_v6) = get_router_load_balancer_ips(router, false) in {
> > > >            if (not all_ips_v4.is_empty()) {
> > > >                LogicalRouterArpFlow(.lr = router,
> > > >                                     .lrp = Some{lrp},
> > > > @@ -5422,21 +5459,14 @@ var residence_check = match (is_redirect) {
> > > >                                     .drop = false,
> > > >                                     .priority = 90,
> > > >                                     .external_ids = map_empty())
> > > > -        }
> > > > -    };
> > > > -    for (RouterLBVIP(.router = &Router{._uuid= lr_uuid}, .vip = vip)) {
> > > > -        Some{(var ip_address, _)} = ip_address_and_port_from_lb_key(vip) in {
> > > > -            IPv6{var ipv6} = ip_address in
> > > > -            LogicalRouterNdFlow(.lr = router,
> > > > -                                .lrp = Some{lrp},
> > > > -                                .action = "nd_na",
> > > > -                                .ip = ipv6,
> > > > -                                .sn_ip = false,
> > > > -                                .mac = rEG_INPORT_ETH_ADDR(),
> > > > -                                .extra_match = residence_check,
> > > > -                                .drop = false,
> > > > -                                .priority = 90,
> > > > -                                .external_ids = map_empty())
> > > > +        };
> > > > +        if (not all_ips_v6.is_empty()) {
> > > > +            LogicalRouterNdFlowLB(.lr = router,
> > > > +                                  .lrp = Some{lrp},
> > > > +                                  .ip = "{ " ++ all_ips_v6.join(", ") ++ " }",
> > > > +                                  .mac = rEG_INPORT_ETH_ADDR(),
> > > > +                                  .extra_match = residence_check,
> > > > +                                  .external_ids = map_empty())
> > > >            }
> > > >        }
> > > >    }
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index 190f78261..b55b01f82 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -1698,13 +1698,10 @@ match=(inport == "lrp" && arp.op == 1 && arp.tpa == { 192.168.2.1, 192.168.2.4,
> > > >    action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > >    match=(inport == "lrp" && ip6.dst == {fe80::200:ff:fe00:1, ff02::1:ff00:1} && nd_ns && nd.target == fe80::200:ff:fe00:1), dnl
> > > > -action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:1; nd.target = fe80::200:ff:fe00:1; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp" && nd_ns && nd.target == fe80::200:ff:fe00:101:8080), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:101:8080; nd.target = fe80::200:ff:fe00:101:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > -  table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp" && nd_ns && nd.target == fe80::200:ff:fe00:102:8080), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:102:8080; nd.target = fe80::200:ff:fe00:102:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +match=(inport == "lrp" && nd_ns && nd.target == { fe80::200:ff:fe00:101:8080, fe80::200:ff:fe00:102:8080 }), dnl
> > > > +action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > >    match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == 43.43.43.1 && arp.spa == 43.43.43.0/24), dnl
> > > >    action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> > > > @@ -1713,13 +1710,10 @@ match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == { 192.168.2.1, 192.16
> > > >    action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > >    match=(inport == "lrp-public" && ip6.dst == {fe80::200:ff:fe00:100, ff02::1:ff00:100} && nd_ns && nd.target == fe80::200:ff:fe00:100), dnl
> > > > -action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:100; nd.target = fe80::200:ff:fe00:100; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > -  table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp-public" && nd_ns && nd.target == fe80::200:ff:fe00:101:8080), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:101:8080; nd.target = fe80::200:ff:fe00:101:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp-public" && nd_ns && nd.target == fe80::200:ff:fe00:102:8080), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:102:8080; nd.target = fe80::200:ff:fe00:102:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +match=(inport == "lrp-public" && nd_ns && nd.target == { fe80::200:ff:fe00:101:8080, fe80::200:ff:fe00:102:8080 }), dnl
> > > > +action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >    ])
> > > >    # xreg0[0..47] isn't used anywhere else.
> > > > @@ -1773,13 +1767,10 @@ match=(inport == "lrp" && arp.op == 1 && arp.tpa == { 192.168.2.1, 192.168.2.4,
> > > >    action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > >    match=(inport == "lrp" && ip6.dst == {fe80::200:ff:fe00:1, ff02::1:ff00:1} && nd_ns && nd.target == fe80::200:ff:fe00:1), dnl
> > > > -action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:1; nd.target = fe80::200:ff:fe00:1; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp" && nd_ns && nd.target == fe80::200:ff:fe00:101:8080), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:101:8080; nd.target = fe80::200:ff:fe00:101:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > -  table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp" && nd_ns && nd.target == fe80::200:ff:fe00:102:8080), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:102:8080; nd.target = fe80::200:ff:fe00:102:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +match=(inport == "lrp" && nd_ns && nd.target == { fe80::200:ff:fe00:101:8080, fe80::200:ff:fe00:102:8080 }), dnl
> > > > +action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > >    match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == 43.43.43.1 && arp.spa == 43.43.43.0/24), dnl
> > > >    action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> > > > @@ -1788,13 +1779,10 @@ match=(inport == "lrp-public" && arp.op == 1 && arp.tpa == { 192.168.2.1, 192.16
> > > >    action=(eth.dst = eth.src; eth.src = xreg0[[0..47]]; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = xreg0[[0..47]]; arp.tpa <-> arp.spa; outport = inport; flags.loopback = 1; output;)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > >    match=(inport == "lrp-public" && ip6.dst == {fe80::200:ff:fe00:100, ff02::1:ff00:100} && nd_ns && nd.target == fe80::200:ff:fe00:100 && is_chassis_resident("cr-lrp-public")), dnl
> > > > -action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:100; nd.target = fe80::200:ff:fe00:100; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > -  table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp-public" && nd_ns && nd.target == fe80::200:ff:fe00:101:8080 && is_chassis_resident("cr-lrp-public")), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:101:8080; nd.target = fe80::200:ff:fe00:101:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +action=(nd_na_router { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >      table=3 (lr_in_ip_input     ), priority=90   , dnl
> > > > -match=(inport == "lrp-public" && nd_ns && nd.target == fe80::200:ff:fe00:102:8080 && is_chassis_resident("cr-lrp-public")), dnl
> > > > -action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = fe80::200:ff:fe00:102:8080; nd.target = fe80::200:ff:fe00:102:8080; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > > +match=(inport == "lrp-public" && nd_ns && nd.target == { fe80::200:ff:fe00:101:8080, fe80::200:ff:fe00:102:8080 } && is_chassis_resident("cr-lrp-public")), dnl
> > > > +action=(nd_na { eth.src = xreg0[[0..47]]; ip6.src = nd.target; nd.tll = xreg0[[0..47]]; outport = inport; flags.loopback = 1; output; };)
> > > >    ])
> > > >    # Priority 91 drop flows (per distributed gw port), if port is not resident.
> > > > 
> > > 
> 


More information about the dev mailing list