> Hi Lorenzo,
>
> I have some minor findings below.
Hi Mark,
thx for the fast review :)
>
[...]
>
> s/,od/, od/
ack, I will fix it
>
> > + io_port, ctrl_meter, stage_hint, where,
> > hash);
> > }
> > /* Adds a row with the specified contents to the Logical_Flow table. */
> > @@ -6870,16 +6880,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> > *op,
> > /* Check if the ovn port has a network configured on which we
> > could
> > * expect ARP requests for the LB VIP.
> > */
> > - if (ip_parse(ip_addr, &ipv4_addr)) {
> > - if (lrouter_port_ipv4_reachable(op, ipv4_addr)) {
> > - build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > - ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
> > - stage_hint);
> > - } else {
> > - build_lswitch_rport_arp_req_flow_for_unreachable_ip(
> > - ip_addr, AF_INET, sw_od, 90, lflows,
> > - stage_hint);
> > - }
> > + if (ip_parse(ip_addr, &ipv4_addr) &&
> > + lrouter_port_ipv4_reachable(op, ipv4_addr)) {
> > + build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > + ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
> > + stage_hint);
> > }
> > }
> > SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v6) {
> > @@ -6888,16 +6893,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> > *op,
> > /* Check if the ovn port has a network configured on which we
> > could
> > * expect NS requests for the LB VIP.
> > */
> > - if (ipv6_parse(ip_addr, &ipv6_addr)) {
> > - if (lrouter_port_ipv6_reachable(op, &ipv6_addr)) {
> > - build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > - ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
> > - stage_hint);
> > - } else {
> > - build_lswitch_rport_arp_req_flow_for_unreachable_ip(
> > - ip_addr, AF_INET6, sw_od, 90, lflows,
> > - stage_hint);
> > - }
> > + if (ipv6_parse(ip_addr, &ipv6_addr) &&
> > + lrouter_port_ipv6_reachable(op, &ipv6_addr)) {
> > + build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > + ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
> > + stage_hint);
> > }
> > }
> > @@ -9422,9 +9422,70 @@ build_lrouter_defrag_flows_for_lb(struct
> > ovn_northd_lb *lb,
> > ds_destroy(&defrag_actions);
> > }
> > +static void
> > +build_lrouter_flows_for_unreachable_ips(struct ovn_northd_lb *lb,
>
> The name of this function is misleading. "build_lrouter_*" typically means
> that we are adding flows to a logical router. In this case we are adding
> flows to logical switches that are connected to logical routers.
ack, I will fix it
>
> > + struct ovn_lb_vip *lb_vip,
> > + struct hmap *lflows,
> > + struct hmap *ports,
> > + struct ds *match,
> > + struct ds *action)
> > +{
> > + bool ipv4 = IN6_IS_ADDR_V4MAPPED(&lb_vip->vip);
> > + ovs_be32 ipv4_addr;
> > +
> > + ds_clear(match);
> > + if (ipv4) {
> > + if (!ip_parse(lb_vip->vip_str, &ipv4_addr)) {
> > + return;
> > + }
> > + ds_put_format(match, "%s && arp.op == 1 && arp.tpa == %s",
> > + FLAGBIT_NOT_VXLAN, lb_vip->vip_str);
> > + } else {
> > + ds_put_format(match, "%s && nd_ns && nd.target == %s",
> > + FLAGBIT_NOT_VXLAN, lb_vip->vip_str);
> > + }
> > +
> > + ds_clear(action);
> > + ds_put_cstr(action, "outport = \""MC_FLOOD"\"; output;");
>
> Since the action is always the same, you could just use a `static const char
> *` for the action instead of `struct ds`.
ack, I will fix it
>
> > + uint32_t hash = ovn_logical_flow_hash(
> > + ovn_stage_get_table(S_SWITCH_IN_L2_LKUP),
> > + ovn_stage_get_pipeline_name(S_SWITCH_IN_L2_LKUP), 90,
> > + ds_cstr(match), ds_cstr(action));
> > +
> > + for (size_t i = 0; i < lb->n_nb_lr; i++) {
>
> The body of this for loop appears to be over-indented by 4 spaces.
ack, I will fix it
>
> > + struct ovn_datapath *od = lb->nb_lr[i];
> > + for (size_t j = 0; j < od->n_router_ports; j++) {
> > + struct ovn_port *op = ovn_port_get_peer(ports,
> > +
> > od->router_ports[j]);
> > + if (!op) {
> > + continue;
> > + }
> > +
> > + struct ovn_port *peer = op->peer;
> > + if (!peer || !peer->nbsp || lsp_is_external(peer->nbsp)) {
> > + continue;
> > + }
> > +
> > + if (!od->is_gw_router && !is_l3dgw_port(op)) {
> > + continue;
> > + }
>
> You could move the od->is_gw_router check outside of this loop. This could
> let you skip the inner loop for all datapaths that are not gateway routers.
I think we can remove this check since we can even run a lrouter with a
distributed gw router port. I guess we can add the following check in the outer
loop:
if (!od->is_gw_router && !od->n_l3dgw_ports) {
continue;
}
Regards,
Lorenzo
>
> > +
> > + if ((ipv4 && lrouter_port_ipv4_reachable(op, ipv4_addr)) ||
> > + (!ipv4 && lrouter_port_ipv6_reachable(op,
> > &lb_vip->vip))) {
> > + continue;
> > + }
> > + ovn_lflow_add_at_with_hash(lflows, peer->od,
> > + S_SWITCH_IN_L2_LKUP, 90,
> > + ds_cstr(match), ds_cstr(action),
> > + NULL, NULL,
> > &peer->nbsp->header_,
> > + OVS_SOURCE_LOCATOR, hash);
> > + }
> > + }
> > +}
> > +
> > static void
> > build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows,
> > - struct shash *meter_groups,
> > + struct hmap *ports, struct shash *meter_groups,
> > struct ds *match, struct ds *action)
> > {
> > if (!lb->n_nb_lr) {
> > @@ -9434,6 +9495,9 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb,
> > struct hmap *lflows,
> > for (size_t i = 0; i < lb->n_vips; i++) {
> > struct ovn_lb_vip *lb_vip = &lb->vips[i];
> > + build_lrouter_flows_for_unreachable_ips(lb, lb_vip, lflows, ports,
> > + match, action);
> > +
> > build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i],
> > lflows, match, action,
> > meter_groups);
> > @@ -12759,7 +12823,7 @@ build_lflows_thread(void *arg)
> > build_lrouter_defrag_flows_for_lb(lb, lsi->lflows,
> > &lsi->match);
> > build_lrouter_flows_for_lb(lb, lsi->lflows,
> > - lsi->meter_groups,
> > + lsi->ports,
> > lsi->meter_groups,
> > &lsi->match,
> > &lsi->actions);
> > build_lswitch_flows_for_lb(lb, lsi->lflows,
> > lsi->meter_groups,
> > @@ -12928,8 +12992,9 @@ build_lswitch_and_lrouter_flows(struct hmap
> > *datapaths, struct hmap *ports,
> > &lsi.actions,
> > &lsi.match);
> > build_lrouter_defrag_flows_for_lb(lb, lsi.lflows, &lsi.match);
> > - build_lrouter_flows_for_lb(lb, lsi.lflows, lsi.meter_groups,
> > - &lsi.match, &lsi.actions);
> > + build_lrouter_flows_for_lb(lb, lsi.lflows, lsi.ports,
> > + lsi.meter_groups, &lsi.match,
> > + &lsi.actions);
> > build_lswitch_flows_for_lb(lb, lsi.lflows, lsi.meter_groups,
> > &lsi.match, &lsi.actions);
> > }
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
