[ovs-dev] [PATCH v6 2/3] Send gateway port ARP through router internal ports

Anil Venkata anilvenkata at redhat.com
Mon Jul 30 12:21:57 UTC 2018


Thanks Mark for the review. Please see my reply inline.


On Thu, Jul 19, 2018 at 7:04 PM, Mark Michelson <mmichels at redhat.com> wrote:

> This patch had some compilation errors. It may be due to changes in master
> since this patchset was posted:
>
> ovn/controller/pinctrl.c: In function ‘send_garp_run’:
> ovn/controller/pinctrl.c:2303:48: error: passing argument 8 of
> ‘get_nat_addresses_and_keys’ discards ‘const’ qualifier from pointer target
> type [-Werror=discarded-qualifiers]
>                                 &nat_addresses, local_datapaths);
>                                                 ^~~~~~~~~~~~~~~
> ovn/controller/pinctrl.c:2182:1: note: expected ‘struct hmap *’ but
> argument is of type ‘const struct hmap *’
>  get_nat_addresses_and_keys(struct ovsdb_idl_index *sbrec_chassis_by_name,
>  ^~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
Thanks, it is a warning and I will fix it.


>
>
> On 06/25/2018 03:53 PM, vkommadi at redhat.com wrote:
>
>> From: venkata anil <vkommadi at redhat.com>
>>
>> External switches should learn the distributed gateway port MAC address
>> as they have to forward the packet tagged with tenant vlan network but
>> with this MAC as destination MAC address. So router has to send ARP
>> reply and gARP for this MAC address through router internal patch ports
>> connecting tenant vlan networks.
>>
>> Signed-off-by: Venkata Anil <vkommadi at redhat.com>
>> ---
>>
>> v5->v6:
>> * Rebased
>>
>> v4->v5:
>> * No changes in this patch
>>
>>   ovn/controller/pinctrl.c | 57 ++++++++++++++++++++++++++++++
>> +++++++++++++-----
>>   ovn/northd/ovn-northd.c  | 57 ++++++++++++++++++++++++++++++
>> ++++++++++++++++++
>>   tests/ovn.at             |  6 +++++
>>   3 files changed, 114 insertions(+), 6 deletions(-)
>>
>> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>> index a0bf602..37157aa 100644
>> --- a/ovn/controller/pinctrl.c
>> +++ b/ovn/controller/pinctrl.c
>> @@ -2185,8 +2185,47 @@ get_nat_addresses_and_keys(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>>                              struct sset *local_l3gw_ports,
>>                              const struct sbrec_chassis *chassis,
>>                              const struct sset *active_tunnels,
>> -                           struct shash *nat_addresses)
>> +                           struct shash *nat_addresses,
>> +                           struct hmap *local_datapaths)
>>   {
>> +    /* When a router has tenant vlan networks, gARP for distributed
>> gateway
>> +     * router port has to be sent through internal tenant vlan network's
>> +     * localnet port, so that external switches can learn this MAC and
>> forward
>> +     * tenant vlan network traffic with distributed gateway router port
>> MAC
>> +     * as destination MAC address */
>> +
>> +    struct local_datapath *ldp;
>> +    struct shash router_vlan_ports;
>> +
>> +    shash_init(&router_vlan_ports);
>> +    HMAP_FOR_EACH (ldp, hmap_node, local_datapaths) {
>> +        const struct sbrec_port_binding *crp;
>> +        crp = ldp->chassisredirect_port;
>> +        /* check if it a router with chassis redirect port,
>> +         * get corresponding distributed port */
>> +        if (crp && crp->chassis &&
>> +            !strcmp(crp->chassis->name, chassis->name)) {
>> +            const struct sbrec_port_binding *dp = NULL;
>> +            for (int i = 0; i < ldp->n_peer_dps; i++) {
>> +                if (!strcmp(ldp->peer_dps[i]->patch->logical_port,
>> +                            smap_get(&crp->options,
>> "distributed-port"))) {
>> +                    dp = ldp->peer_dps[i]->peer;
>> +                    break;
>> +                }
>> +            }
>> +
>>
>
> It seems possible to reach this point and have dp be NULL. You probably
> should continue the hmap traversal if dp is NULL.


No need. As I said in my previous patch, chassisredirect port is created
from distributed router port and doesn't exist without distributed port.


>
>
> +            /* Save router internal port (patch port on tenant vlan
>> network)
>> +             * along with distributed port. */
>> +            for (int i = 0; i < ldp->n_peer_dps; i++) {
>> +                if (strcmp(ldp->peer_dps[i]->patch->logical_port,
>> +                           smap_get(&crp->options, "distributed-port")))
>> {
>> +                    shash_add(&router_vlan_ports,
>> +                              ldp->peer_dps[i]->peer->logical_port, dp);
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>>       const char *gw_port;
>>       SSET_FOR_EACH(gw_port, local_l3gw_ports) {
>>           const struct sbrec_port_binding *pb;
>> @@ -2196,11 +2235,16 @@ get_nat_addresses_and_keys(struct
>> ovsdb_idl_index *sbrec_chassis_by_name,
>>               continue;
>>           }
>>   -        if (pb->n_nat_addresses) {
>> -            for (int i = 0; i < pb->n_nat_addresses; i++) {
>> +        /* Router internatl ports should send gARP for distributed port
>>
>
> s/internatl/internal/
>
>
>
sure.



> +         * NAT addresses */
>> +        const struct sbrec_port_binding *dp;
>> +        dp = shash_find_data(&router_vlan_ports, pb->logical_port);
>> +        const struct sbrec_port_binding *nat_port = dp ? dp : pb;
>> +        if (nat_port->n_nat_addresses) {
>> +            for (int i = 0; i < nat_port->n_nat_addresses; i++) {
>>                   consider_nat_address(sbrec_chassis_by_name,
>>                                        sbrec_port_binding_by_name,
>> -                                     pb->nat_addresses[i], pb,
>> +                                     nat_port->nat_addresses[i], pb,
>>                                        nat_address_keys, chassis,
>>                                        active_tunnels,
>>                                        nat_addresses);
>> @@ -2208,7 +2252,7 @@ get_nat_addresses_and_keys(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>>           } else {
>>               /* Continue to support options:nat-addresses for version
>>                * upgrade. */
>> -            const char *nat_addresses_options = smap_get(&pb->options,
>> +            const char *nat_addresses_options =
>> smap_get(&nat_port->options,
>>
>>  "nat-addresses");
>>               if (nat_addresses_options) {
>>                   consider_nat_address(sbrec_chassis_by_name,
>> @@ -2220,6 +2264,7 @@ get_nat_addresses_and_keys(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>>               }
>>           }
>>       }
>> +    shash_destroy(&router_vlan_ports);
>>   }
>>     static void
>> @@ -2255,7 +2300,7 @@ send_garp_run(struct ovsdb_idl_index
>> *sbrec_chassis_by_name,
>>                                  sbrec_port_binding_by_name,
>>                                  &nat_ip_keys, &local_l3gw_ports,
>>                                  chassis, active_tunnels,
>> -                               &nat_addresses);
>> +                               &nat_addresses, local_datapaths);
>>       /* For deleted ports and deleted nat ips, remove from
>> send_garp_data. */
>>       struct shash_node *iter, *next;
>>       SHASH_FOR_EACH_SAFE (iter, next, &send_garp_data) {
>> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> index 350f015..eac889f 100644
>> --- a/ovn/northd/ovn-northd.c
>> +++ b/ovn/northd/ovn-northd.c
>> @@ -5013,6 +5013,63 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> hmap *ports,
>>                             ds_cstr(&match), ds_cstr(&actions));
>>           }
>>   +        /* ARP requests for distributed port IP address but coming
>> from router
>> +         * internal network vlan, should be replied through router
>> internal
>> +         * network vlan ports */
>> +        if (op->od->l3dgw_port && op == op->od->l3dgw_port
>> +            && op->od->l3redirect_port) {
>> +            for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
>> +                ds_clear(&match);
>> +                ds_put_format(&match,
>> +                              "flags.rcv_from_vlan == 1 && "
>> +                              "arp.tpa == %s && arp.op == 1 && "
>> +                              "is_chassis_resident(%s)",
>> +                               op->lrp_networks.ipv4_addrs[i].addr_s,
>> +                               op->od->l3redirect_port->json_key);
>> +
>> +                ds_clear(&actions);
>> +                ds_put_format(&actions,
>> +                    "eth.dst = eth.src; "
>> +                    "eth.src = %s; "
>> +                    "arp.op = 2; /* ARP reply */ "
>> +                    "arp.tha = arp.sha; "
>> +                    "arp.sha = %s; "
>> +                    "arp.tpa = arp.spa; "
>> +                    "arp.spa = %s; "
>> +                    "flags.loopback = 1; ",
>> +                    op->lrp_networks.ea_s,
>> +                    op->lrp_networks.ea_s,
>> +                    op->lrp_networks.ipv4_addrs[i].addr_s);
>> +
>> +                /* Add internal vlan network ports as output ports */
>> +                bool router_ports_exist = false;
>> +                struct ovn_datapath *dp;
>> +                HMAP_FOR_EACH (dp, key_node, datapaths) {
>> +                    if (!dp->nbs) {
>> +                        continue;
>> +                    }
>> +                    if (!dp->localnet_port) {
>> +                        continue;
>> +                    }
>> +                    for (size_t j = 0; j < dp->n_router_ports; j++) {
>> +                        struct ovn_port *rp = dp->router_ports[j];
>> +                        if (rp->peer && rp->peer->od == op->od &&
>> +                            rp->peer != op) {
>> +                            router_ports_exist = true;
>> +                            ds_put_format(&actions,
>> +                                "outport = %s; "
>> +                                "output;",
>> +                                rp->peer->json_key);
>> +                        }
>> +                    }
>> +                }
>> +                if (router_ports_exist) {
>> +                    ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT,
>> 90,
>> +                        ds_cstr(&match), ds_cstr(&actions));
>> +                }
>> +            }
>> +        }
>> +
>>           /* A set to hold all load-balancer vips that need ARP
>> responses. */
>>           struct sset all_ips = SSET_INITIALIZER(&all_ips);
>>           int addr_family;
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 5ae767d..ae04a07 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -7889,6 +7889,12 @@ foo_mac="000001010203"
>>   gw_mac="000002010203"
>>   nexthop_mac="f00000010204"
>>   +# gARP for distributed port has to be sent through router internal
>> patch port
>> +# which is connected to vlan network
>> +garp=ffffffffffff${gw_mac}8100000208060001080006040001${gw_
>> mac}${gw_ip}000000000000${gw_ip}
>> +echo $garp >> hv1-br-ex_n2-rx.expected
>> +OVN_CHECK_PACKETS([hv1/br-ex_n2-rx.pcap], [hv1-br-ex_n2-rx.expected])
>> +
>>   # Send ip packet from foo1 to 8.8.8.8
>>   src_mac="f00000010203"
>>   dst_mac="000001010203"
>>
>>
>


More information about the dev mailing list