[ovs-dev] [PATCH 9/9] ovn-controller: Drop most uses of OVS patch ports.

Mickey Spiegel mickeys.dev at gmail.com
Mon Dec 5 20:58:24 UTC 2016


On Sun, Dec 4, 2016 at 11:17 PM, Ben Pfaff <blp at ovn.org> wrote:

> Until now, ovn-controller has implemented OVN logical patch ports and
> l3gateway ports in terms of OVS patch ports.  It is a hassle to create and
> destroy ports, and it is also wasteful compared to what the patch ports
> actually buy us: the ability to "save and restore" a packet around a
> recursive trip through the flow table.  The "clone" action can do that too,
> without the need to create a port.  This commit takes advantage of the
> clone action for that purpose, getting rid of most of the patch ports
> previously created by ovn-controller.
>

I like it. It took me a bit to change my mindset from most physical.c flows
based on local ports with ofport values, splitting into some local ports
with ofport values and other local ports accessing the same refactored
code directly from a walk of sbrec_port_binding, based on port type.

This will allow me to cut a bunch of code from the first chassisredirect
patch in my distributed NAT patch set, similar to the way you cut
add_logical_path_ports in patch.c. More importantly for the distributed
NAT patch set, this solves most of my egress loopback open issue.
The local_datapath changes earlier in the patch set probably solve
my localnet issue. I need to look at that a little more carefully.

A few comments below, mostly about code organization. One real
technical issue.

After this, I will go back to the first patch and work my way through
patch by patch.


> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>  ovn/controller/binding.c        |   2 +-
>  ovn/controller/ovn-controller.c |   8 ++--
>  ovn/controller/patch.c          | 101 +++++++++++-------------------
> ----------
>  ovn/controller/patch.h          |   3 +-
>  ovn/controller/physical.c       |  96 +++++++++++++++++++++++++++++-
> --------
>  ovn/controller/physical.h       |   7 +--
>  ovn/controller/pinctrl.c        |  36 ++++++++------
>  ovn/controller/pinctrl.h        |   4 +-
>  tests/ovn-controller.at         |  57 ++---------------------
>  9 files changed, 141 insertions(+), 173 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index b53af98..2759e23 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -391,7 +391,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>                                         "l3gateway-chassis");
>          if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) {
>              add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> -                               false, local_datapaths);
> +                               true, local_datapaths);
>          }
>      } else if (chassis_rec && binding_rec->chassis == chassis_rec) {
>          if (ctx->ovnsb_idl_txn) {
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-
> controller.c
> index ff48a94..7d372b4 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -515,12 +515,12 @@ main(int argc, char *argv[])
>          }
>
>          if (br_int && chassis) {
> -            patch_run(&ctx, br_int, chassis_id, &local_datapaths);
> +            patch_run(&ctx, br_int, chassis, &local_datapaths);
>
>              enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
>
> &pending_ct_zones);
>
> -            pinctrl_run(&ctx, &lports, br_int, chassis_id,
> &local_datapaths);
> +            pinctrl_run(&ctx, &lports, br_int, chassis, &local_datapaths);
>              update_ct_zones(&all_lports, &local_datapaths, &ct_zones,
>                              ct_zone_bitmap, &pending_ct_zones);
>              if (ctx.ovs_idl_txn) {
> @@ -531,8 +531,8 @@ main(int argc, char *argv[])
>                            &group_table, &ct_zones, &flow_table);
>
>                  physical_run(&ctx, mff_ovn_geneve,
> -                             br_int, chassis_id, &ct_zones, &flow_table,
> -                             &local_datapaths);
> +                             br_int, chassis, &ct_zones, &lports,
> +                             &flow_table, &local_datapaths);
>
>                  ofctrl_put(&flow_table, &pending_ct_zones,
>                             get_nb_cfg(ctx.ovnsb_idl));
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> index ce82b89..9893c79 100644
> --- a/ovn/controller/patch.c
> +++ b/ovn/controller/patch.c
> @@ -137,7 +137,7 @@ add_bridge_mappings(struct controller_ctx *ctx,
>                      const struct ovsrec_bridge *br_int,
>                      struct shash *existing_ports,
>                      struct hmap *local_datapaths,
> -                    const char *chassis_id)
> +                    const struct sbrec_chassis *chassis)
>  {
>      /* Get ovn-bridge-mappings. */
>      const char *mappings_cfg = "";
> @@ -207,7 +207,7 @@ add_bridge_mappings(struct controller_ctx *ctx,
>              patch_port_id = "ovn-localnet-port";
>          } else if (!strcmp(binding->type, "l2gateway")) {
>              if (!binding->chassis
> -                || strcmp(chassis_id, binding->chassis->name)) {
> +                || strcmp(chassis->name, binding->chassis->name)) {
>                  /* This L2 gateway port is not bound to this chassis,
>                   * so we should not create any patch ports for it. */
>                  continue;
> @@ -247,86 +247,39 @@ add_bridge_mappings(struct controller_ctx *ctx,
>      shash_destroy(&bridge_mappings);
>  }
>
> -/* Add one OVS patch port for each OVN logical patch port.
> - *
> - * It's wasteful to create an OVS patch port per OVN logical patch port,
> when
> - * really there's no benefit to them beyond a way to identify how a packet
> - * ingressed into a logical datapath.
> - *
> - * There are two obvious ways to improve the situation here, by modifying
> - * OVS:
> - *
> - *     1. Add a way to configure in OVS which fields are preserved on a
> hop
> - *        across an OVS patch port.  If MFF_LOG_DATAPATH and
> MFF_LOG_INPORT
> - *        were preserved, then only a single pair of OVS patch ports
> would be
> - *        required regardless of the number of OVN logical patch ports.
> - *
> - *     2. Add a new OpenFlow extension action modeled on "resubmit" that
> also
> - *        saves and restores the packet data and metadata (the inability
> to do
> - *        this is the only reason that "resubmit" can't be used
> already).  Or
> - *        add OpenFlow extension actions to otherwise save and restore
> packet
> - *        data and metadata.
> - */
> -static void
> -add_logical_patch_ports(struct controller_ctx *ctx,
> -                        const struct ovsrec_bridge *br_int,
> -                        const char *local_chassis_id,
> -                        struct hmap *local_datapaths,
> -                        struct shash *existing_ports)
> +void
> +patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> +          const struct sbrec_chassis *chassis, struct hmap
> *local_datapaths)
>  {
> -    const struct sbrec_chassis *chassis_rec;
> -    chassis_rec = get_chassis(ctx->ovnsb_idl, local_chassis_id);
> -    if (!chassis_rec) {
> -        return;
> -    }
> -
> -    const struct local_datapath *ld;
> -    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> -        for (size_t i = 0; i < ld->ldatapath->n_lports; i++) {
> -            const struct sbrec_port_binding *pb =
> ld->ldatapath->lports[i];
> -            const char *patch_port_id = "ovn-logical-patch-port";
> -
> -            bool is_local_l3gateway = false;
> -            if (!strcmp(pb->type, "l3gateway")) {
> -                const char *chassis = smap_get(&pb->options,
> -                                               "l3gateway-chassis");
> -                if (chassis && !strcmp(local_chassis_id, chassis)) {
> -                    is_local_l3gateway = true;
> -                    patch_port_id = "ovn-l3gateway-port";
> -                    if (pb->chassis != chassis_rec && ctx->ovnsb_idl_txn)
> {
> -                        sbrec_port_binding_set_chassis(pb, chassis_rec);
> -                    }
> -                }
> -            }
> -
> -            if (!strcmp(pb->type, "patch") || is_local_l3gateway) {
> -                const char *local = pb->logical_port;
> -                const char *peer = smap_get(&pb->options, "peer");
> -                if (!peer) {
> -                    continue;
> +    /* Bind l3gateway ports to chassis.
> +     *
> +     * (It would make sense for ovn-northd to do this, but if it is ever
> +     * modified to do so, ovn-controller still needs to do the binding as
> long
> +     * as OVN supports upgrades against older ovn-northd.) */
> +    if (ctx->ovnsb_idl_txn && chassis) {
> +        const struct sbrec_port_binding *binding;
> +        SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
> +            if (!strcmp(binding->type, "l3gateway")) {
> +                const char *l3gw_chassis = smap_get(&binding->options,
> +                                                    "l3gateway-chassis");
> +                if (!strcmp(l3gw_chassis, chassis->name)
> +                    && binding->chassis != chassis) {
> +                    sbrec_port_binding_set_chassis(binding, chassis);
>

I prefer that sbrec_port_binding_set_chassis be handled in binding.c
rather than patch.c. For l2gateway, even though patch port creation
is handled in patch.c, sbrec_port_binding_set_chassis is handled in
binding.c. There is already l3gateway code in binding.c to trigger
add_local_datapath. This code can go in right alongside that.


>                  }
> -
> -                char *src_name = patch_port_name(local, peer);
> -                char *dst_name = patch_port_name(peer, local);
> -                create_patch_port(ctx, patch_port_id, local,
> -                                  br_int, src_name, br_int, dst_name,
> -                                  existing_ports);
> -                free(dst_name);
> -                free(src_name);
>              }
>          }
>      }
> -}
>
> -void
> -patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
> -          const char *chassis_id, struct hmap *local_datapaths)
> -{
>      if (!ctx->ovs_idl_txn) {
>          return;
>      }
>
> -    /* Figure out what patch ports already exist. */
> +    /* Figure out what patch ports already exist.
> +     *
> +     * ovn-controller does not create or use ports of type
> "ovn-l3gateway-port"
> +     * or "ovn-logical-patch-port", but older version did.  We still
> recognize
> +     * them here, so that we delete them at the end of this function, to
> avoid
> +     * leaving useless ports on upgrade. */
>      struct shash existing_ports = SHASH_INITIALIZER(&existing_ports);
>      const struct ovsrec_port *port;
>      OVSREC_PORT_FOR_EACH (port, ctx->ovs_idl) {
> @@ -342,9 +295,7 @@ patch_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
>       * 'existing_ports' any patch ports that do exist in the database and
>       * should be there. */
>      add_bridge_mappings(ctx, br_int, &existing_ports, local_datapaths,
> -                        chassis_id);
> -    add_logical_patch_ports(ctx, br_int, chassis_id, local_datapaths,
> -                            &existing_ports);
> +                        chassis);
>
>      /* Now 'existing_ports' only still contains patch ports that exist in
> the
>       * database but shouldn't.  Delete them from the database. */
> diff --git a/ovn/controller/patch.h b/ovn/controller/patch.h
> index 12ebeb9..2feb44b 100644
> --- a/ovn/controller/patch.h
> +++ b/ovn/controller/patch.h
> @@ -25,8 +25,9 @@
>  struct controller_ctx;
>  struct hmap;
>  struct ovsrec_bridge;
> +struct sbrec_chassis;
>
>  void patch_run(struct controller_ctx *, const struct ovsrec_bridge
> *br_int,
> -               const char *chassis_id, struct hmap *local_datapaths);
> +               const struct sbrec_chassis *, struct hmap
> *local_datapaths);
>
>  #endif /* ovn/patch.h */
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 5de3530..747bb13 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -18,6 +18,7 @@
>  #include "byte-order.h"
>  #include "flow.h"
>  #include "lflow.h"
> +#include "lport.h"
>  #include "lib/poll-loop.h"
>  #include "ofctrl.h"
>  #include "openvswitch/hmap.h"
> @@ -286,8 +287,10 @@ load_logical_ingress_metadata(const struct
> sbrec_port_binding *binding,
>  static void
>  consider_port_binding(enum mf_field_id mff_ovn_geneve,
>                        const struct simap *ct_zones,
> +                      const struct lport_index *lports,
>                        struct hmap *local_datapaths,
>                        const struct sbrec_port_binding *binding,
> +                      const struct sbrec_chassis *chassis,
>                        struct ofpbuf *ofpacts_p,
>                        struct hmap *flow_table)
>  {
> @@ -297,14 +300,76 @@ consider_port_binding(enum mf_field_id
> mff_ovn_geneve,
>          return;
>      }
>
> +    struct match match;
> +    if (!strcmp(binding->type, "patch")
> +        || (!strcmp(binding->type, "l3gateway")
> +            && binding->chassis == chassis)) {
> +        const char *peer_name = smap_get(&binding->options, "peer");
> +        if (!peer_name) {
> +            return;
> +        }
> +
> +        const struct sbrec_port_binding *peer = lport_lookup_by_name(
> +            lports, peer_name);
> +        if (!peer || strcmp(peer->type, binding->type)) {
> +            return;
> +        }
> +        const char *peer_peer_name = smap_get(&peer->options, "peer");
> +        if (!peer_peer_name || strcmp(peer_peer_name,
> binding->logical_port)) {
> +            return;
> +        }
> +
> +        struct zone_ids zone_ids = { .ct = 0 };
> +        if (!strcmp(binding->type, "l3gateway")) {
> +            char *dnat = alloc_nat_zone_key(&binding->
> datapath->header_.uuid,
> +                                            "dnat");
> +            zone_ids.dnat = simap_get(ct_zones, dnat);
> +            free(dnat);
> +
> +            char *snat = alloc_nat_zone_key(&binding->
> datapath->header_.uuid,
> +                                            "snat");
> +            zone_ids.snat = simap_get(ct_zones, snat);
> +            free(snat);
> +        }
>

For distributed NAT, most of the optimization above compared to
get_zone_ids will go away. I will need dnat and snat zones for
distributed routers as well as gateway routers.

+
> +        put_local_common_flows(dp_key, port_key, false, &zone_ids,
> +                               ofpacts_p, flow_table);
> +
> +        match_init_catchall(&match);
> +        ofpbuf_clear(ofpacts_p);
> +        match_set_metadata(&match, htonll(dp_key));
> +        match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
> +
>

When I implement egress loopback for distributed NAT, while I will
only need egress loopback for patch ports, I should probably code
it up generically for all port types. In order to do that, I will need to
access the code starting here, down to ofpact_finish_CLONE, from
another place. I can take care of refactoring this chunk of code in
that patch if you do not do it here. The difference will be that you
are calling load_logical_ingress_metadata based on peer,
whereas for egress loopback this will be based on binding itself.
Alternatively, I could create my own copy just to optimize since I
do not need to reset metadata (datapath) or any of the three
zones.


> +        size_t clone_ofs = ofpacts_p->size;
> +        struct ofpact_clone *clone = ofpact_put_CLONE(ofpacts_p);
> +        put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
> +        put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
> +        put_load(0, MFF_LOG_CT_ZONE, 0, 32, ofpacts_p);
> +        load_logical_ingress_metadata(peer, &zone_ids, ofpacts_p);
>

Don't you need to recompute the zone_ids based on the peer rather
than based on binding?
The peer is in a different datapath.

Mickey

+        put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p);
> +        put_load(0, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
> +        for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> +            put_load(0, MFF_LOG_REG0 + i, 0, 32, ofpacts_p);
> +        }
> +        put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
> +        put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
> +        clone = ofpbuf_at_assert(ofpacts_p, clone_ofs, sizeof *clone);
> +        ofpacts_p->header = clone;
> +        ofpact_finish_CLONE(ofpacts_p, &clone);
> +
> +        ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
> +                        &match, ofpacts_p);
> +        return;
> +    }
> +
>      /* Find the OpenFlow port for the logical port, as 'ofport'.  This is
>       * one of:
>       *
>       *     - If the port is a VIF on the chassis we're managing, the
>       *       OpenFlow port for the VIF.  'tun' will be NULL.
>       *
> -     *       The same logic handles logical patch ports, as well as
> -     *       localnet patch ports.
> +     *       The same logic handles ports that OVN implements as Open
> vSwitch
> +     *       patch ports, that is, "localnet" and "l2gateway" ports.
>       *
>       *       For a container nested inside a VM and accessible via a VLAN,
>       *       'tag' is the VLAN ID; otherwise 'tag' is 0.
> @@ -365,7 +430,6 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
>          }
>      }
>
> -    struct match match;
>      if (!is_remote) {
>          /* Packets that arrive from a vif can belong to a VM or
>           * to a container located inside that VM. Packets that
> @@ -647,9 +711,10 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
>
>  void
>  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
> -             const struct ovsrec_bridge *br_int, const char
> *this_chassis_id,
> -             const struct simap *ct_zones, struct hmap *flow_table,
> -             struct hmap *local_datapaths)
> +             const struct ovsrec_bridge *br_int,
> +             const struct sbrec_chassis *chassis,
> +             const struct simap *ct_zones, struct lport_index *lports,
> +             struct hmap *flow_table, struct hmap *local_datapaths)
>  {
>
>      /* This bool tracks physical mapping changes. */
> @@ -667,7 +732,7 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
>
>          const char *chassis_id = smap_get(&port_rec->external_ids,
>                                            "ovn-chassis-id");
> -        if (chassis_id && !strcmp(chassis_id, this_chassis_id)) {
> +        if (chassis_id && !strcmp(chassis_id, chassis->name)) {
>              continue;
>          }
>
> @@ -675,10 +740,6 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
>                                          "ovn-localnet-port");
>          const char *l2gateway = smap_get(&port_rec->external_ids,
>                                          "ovn-l2gateway-port");
> -        const char *l3gateway = smap_get(&port_rec->external_ids,
> -                                        "ovn-l3gateway-port");
> -        const char *logpatch = smap_get(&port_rec->external_ids,
> -                                        "ovn-logical-patch-port");
>
>          for (int j = 0; j < port_rec->n_interfaces; j++) {
>              const struct ovsrec_interface *iface_rec =
> port_rec->interfaces[j];
> @@ -703,14 +764,6 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
>                  /* L2 gateway patch ports can be handled just like VIFs.
> */
>                  simap_put(&new_localvif_to_ofport, l2gateway, ofport);
>                  break;
> -            } else if (is_patch && l3gateway) {
> -                /* L3 gateway patch ports can be handled just like VIFs.
> */
> -                simap_put(&new_localvif_to_ofport, l3gateway, ofport);
> -                break;
> -            } else if (is_patch && logpatch) {
> -                /* Logical patch ports can be handled just like VIFs. */
> -                simap_put(&new_localvif_to_ofport, logpatch, ofport);
> -                break;
>              } else if (chassis_id) {
>                  enum chassis_tunnel_type tunnel_type;
>                  if (!strcmp(iface_rec->type, "geneve")) {
> @@ -799,8 +852,9 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
>       * 64 for logical-to-physical translation. */
>      const struct sbrec_port_binding *binding;
>      SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
> -        consider_port_binding(mff_ovn_geneve, ct_zones, local_datapaths,
> -                              binding, &ofpacts, flow_table);
> +        consider_port_binding(mff_ovn_geneve, ct_zones, lports,
> +                              local_datapaths, binding, chassis,
> +                              &ofpacts, flow_table);
>      }
>
>      /* Handle output to multicast groups, in tables 32 and 33. */
> diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
> index ab8a9d2..e2debed 100644
> --- a/ovn/controller/physical.h
> +++ b/ovn/controller/physical.h
> @@ -42,8 +42,9 @@ struct simap;
>
>  void physical_register_ovs_idl(struct ovsdb_idl *);
>  void physical_run(struct controller_ctx *, enum mf_field_id
> mff_ovn_geneve,
> -                  const struct ovsrec_bridge *br_int, const char
> *chassis_id,
> -                  const struct simap *ct_zones, struct hmap *flow_table,
> -                  struct hmap *local_datapaths);
> +                  const struct ovsrec_bridge *br_int,
> +                  const struct sbrec_chassis *chassis,
> +                  const struct simap *ct_zones, struct lport_index *,
> +                  struct hmap *flow_table, struct hmap *local_datapaths);
>
>  #endif /* ovn/physical.h */
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index db9e441..4accf69 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -68,7 +68,7 @@ static void init_send_garps(void);
>  static void destroy_send_garps(void);
>  static void send_garp_wait(void);
>  static void send_garp_run(const struct ovsrec_bridge *,
> -                          const char *chassis_id,
> +                          const struct sbrec_chassis *,
>                            const struct lport_index *lports,
>                            struct hmap *local_datapaths);
>  static void pinctrl_handle_nd_na(const struct flow *ip_flow,
> @@ -755,7 +755,7 @@ pinctrl_recv(const struct ofp_header *oh, enum ofptype
> type)
>  void
>  pinctrl_run(struct controller_ctx *ctx, const struct lport_index *lports,
>              const struct ovsrec_bridge *br_int,
> -            const char *chassis_id,
> +            const struct sbrec_chassis *chassis,
>              struct hmap *local_datapaths)
>  {
>      char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int->name);
> @@ -791,7 +791,7 @@ pinctrl_run(struct controller_ctx *ctx, const struct
> lport_index *lports,
>      }
>
>      run_put_mac_bindings(ctx, lports);
> -    send_garp_run(br_int, chassis_id, lports, local_datapaths);
> +    send_garp_run(br_int, chassis, lports, local_datapaths);
>  }
>
>  void
> @@ -1148,7 +1148,7 @@ send_garp(struct garp_data *garp, long long int
> current_time)
>  /* Get localnet vifs, local l3gw ports and ofport for localnet patch
> ports. */
>  static void
>  get_localnet_vifs_l3gwports(const struct ovsrec_bridge *br_int,
> -                  const char *this_chassis_id,
> +                  const struct sbrec_chassis *chassis,
>                    const struct lport_index *lports,
>                    struct hmap *local_datapaths,
>                    struct sset *localnet_vifs,
> @@ -1162,13 +1162,11 @@ get_localnet_vifs_l3gwports(const struct
> ovsrec_bridge *br_int,
>          }
>          const char *chassis_id = smap_get(&port_rec->external_ids,
>                                            "ovn-chassis-id");
> -        if (chassis_id && !strcmp(chassis_id, this_chassis_id)) {
> +        if (chassis_id && !strcmp(chassis_id, chassis->name)) {
>              continue;
>          }
>          const char *localnet = smap_get(&port_rec->external_ids,
>                                          "ovn-localnet-port");
> -        const char *l3_gateway_port = smap_get(&port_rec->external_ids,
> -                                               "ovn-l3gateway-port");
>          for (int j = 0; j < port_rec->n_interfaces; j++) {
>              const struct ovsrec_interface *iface_rec =
> port_rec->interfaces[j];
>              if (!iface_rec->n_ofport) {
> @@ -1182,10 +1180,6 @@ get_localnet_vifs_l3gwports(const struct
> ovsrec_bridge *br_int,
>                  simap_put(localnet_ofports, localnet, ofport);
>                  continue;
>              }
> -            if (l3_gateway_port) {
> -                sset_add(local_l3gw_ports, l3_gateway_port);
> -                continue;
> -            }
>              const char *iface_id = smap_get(&iface_rec->external_ids,
>                                              "iface-id");
>              if (!iface_id) {
> @@ -1204,6 +1198,21 @@ get_localnet_vifs_l3gwports(const struct
> ovsrec_bridge *br_int,
>              }
>          }
>      }
> +
> +    const struct local_datapath *ld;
> +    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> +        if (!ld->has_local_l3gateway) {
> +            continue;
> +        }
> +
> +        for (size_t i = 0; i < ld->ldatapath->n_lports; i++) {
> +            const struct sbrec_port_binding *pb =
> ld->ldatapath->lports[i];
> +            if (!strcmp(pb->type, "l3gateway")
> +                /* && it's on this chassis */) {
> +                sset_add(local_l3gw_ports, pb->logical_port);
> +            }
> +        }
> +    }
>  }
>
>  static void
> @@ -1248,7 +1257,8 @@ send_garp_wait(void)
>  }
>
>  static void
> -send_garp_run(const struct ovsrec_bridge *br_int, const char *chassis_id,
> +send_garp_run(const struct ovsrec_bridge *br_int,
> +              const struct sbrec_chassis *chassis,
>                const struct lport_index *lports,
>                struct hmap *local_datapaths)
>  {
> @@ -1260,7 +1270,7 @@ send_garp_run(const struct ovsrec_bridge *br_int,
> const char *chassis_id,
>
>      shash_init(&nat_addresses);
>
> -    get_localnet_vifs_l3gwports(br_int, chassis_id, lports,
> local_datapaths,
> +    get_localnet_vifs_l3gwports(br_int, chassis, lports, local_datapaths,
>                        &localnet_vifs, &localnet_ofports,
> &local_l3gw_ports);
>
>      get_nat_addresses_and_keys(&nat_ip_keys, &local_l3gw_ports, lports,
> diff --git a/ovn/controller/pinctrl.h b/ovn/controller/pinctrl.h
> index 553eace..af3c4b0 100644
> --- a/ovn/controller/pinctrl.h
> +++ b/ovn/controller/pinctrl.h
> @@ -25,11 +25,11 @@ struct hmap;
>  struct lport_index;
>  struct ovsrec_bridge;
>  struct controller_ctx;
> +struct sbrec_chassis;
>
>  void pinctrl_init(void);
>  void pinctrl_run(struct controller_ctx *, const struct lport_index *,
> -                 const struct ovsrec_bridge *,
> -                 const char *chassis_id,
> +                 const struct ovsrec_bridge *, const struct sbrec_chassis
> *,
>                   struct hmap *local_datapaths);
>  void pinctrl_wait(struct controller_ctx *);
>  void pinctrl_destroy(void);
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 8ad8f67..2b28279 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -88,8 +88,8 @@ check_patches \
>
>  # Add logical patch ports to connect new logical datapath.
>  #
> -# (Also add a vif on this hypervisor on one of the datapaths,
> -# otherwise the hypervisor will ignore both of them.)
> +# OVN no longer uses OVS patch ports to implement logical patch ports, so
> +# the set of OVS patch ports doesn't change.
>  AT_CHECK([ovn-sbctl \
>      -- --id=@dp1 create Datapath_Binding tunnel_key=1 \
>      -- --id=@dp2 create Datapath_Binding tunnel_key=2 \
> @@ -105,66 +105,17 @@ AT_CHECK([ovn-sbctl \
>  ovs-vsctl add-port br-int dp1vif -- set Interface dp1vif
> external_ids:iface-id=dp1vif
>  check_patches \
>      'br-int  patch-br-int-to-localnet2 patch-localnet2-to-br-int' \
> -    'br-eth0 patch-localnet2-to-br-int patch-br-int-to-localnet2' \
> -    'br-int  patch-foo-to-bar        patch-bar-to-foo' \
> -    'br-int  patch-bar-to-foo        patch-foo-to-bar'
> +    'br-eth0 patch-localnet2-to-br-int patch-br-int-to-localnet2'
>
> -# Delete the mapping and the ovn-bridge-mapping patch ports should go
> away;
> -# the ones from the logical patch port remain.
> +# Delete the mapping and the ovn-bridge-mapping patch ports should go
> away.
>  AT_CHECK([ovs-vsctl remove Open_vSwitch . external-ids
> ovn-bridge-mappings])
>  check_bridge_mappings
> -check_patches \
> -    'br-int patch-foo-to-bar patch-bar-to-foo' \
> -    'br-int patch-bar-to-foo patch-foo-to-bar'
> -
> -# Change name of logical patch port, check that the OVS patch ports
> -# get updated.
> -AT_CHECK([ovn-sbctl \
> -    -- set Port_Binding foo logical_port=quux options:peer=baz \
> -    -- set Port_Binding bar logical_port=baz  options:peer=quux])
> -check_patches \
> -    'br-int patch-quux-to-baz patch-baz-to-quux' \
> -    'br-int patch-baz-to-quux patch-quux-to-baz'
> -
> -# Drop the vif from the patched-together datapaths and verify that the
> patch
> -# ports disappear.
> -AT_CHECK([ovs-vsctl del-port dp1vif])
> -check_patches
> -
> -# Put the vif back and the patch ports reappear.
> -AT_CHECK([ovs-vsctl add-port br-int dp1vif -- set Interface dp1vif
> external_ids:iface-id=dp1vif])
> -check_patches \
> -    'br-int patch-quux-to-baz patch-baz-to-quux' \
> -    'br-int patch-baz-to-quux patch-quux-to-baz'
> -
> -# Create an empty database, serve it and switch to it
> -# and verify that the OVS patch ports disappear
> -# then put it back and verify that they reappear
> -
> -on_exit 'kill `cat $ovs_base/ovn-sb/ovsdb-server-2.pid`'
> -
> -ovsdb-tool create $ovs_base/ovn-sb/ovn-sb1.db
> "$abs_top_srcdir"/ovn/ovn-sb.ovsschema
> -as ovn-sb ovsdb-server --detach --pidfile=$ovs_base/ovn-sb/ovsdb-server-2.pid
> --remote=punix:$ovs_base/ovn-sb/ovn-sb1.sock $ovs_base/ovn-sb/ovn-sb1.db \
> -   --unixctl=$ovs_base/ovn-sb/ovsdb-server-2.ctl
> -AT_CHECK([ovs-vsctl -- set Open_vSwitch . external-ids:ovn-remote=unix:$
> ovs_base/ovn-sb/ovn-sb1.sock])
> -check_patches
> -AT_CHECK([ovs-vsctl -- set Open_vSwitch . external-ids:ovn-remote=unix:$
> ovs_base/ovn-sb/ovn-sb.sock])
> -check_patches \
> -    'br-int patch-quux-to-baz patch-baz-to-quux' \
> -    'br-int patch-baz-to-quux patch-quux-to-baz'
> -
> -# Change the logical patch ports to VIFs and verify that the OVS patch
> -# ports disappear.
> -AT_CHECK([ovn-sbctl \
> -    -- set Port_Binding quux type='""' options='{}' \
> -    -- set Port_Binding baz type='""' options='{}'])
>  check_patches
>
>  # Gracefully terminate daemons
>  OVN_CLEANUP_SBOX([hv])
>  OVN_CLEANUP_VSWITCH([main])
>  as ovn-sb
> -OVS_APP_EXIT_AND_WAIT_BY_TARGET([$ovs_base/ovn-sb/ovsdb-server-2.ctl],
> [$ovs_base/ovn-sb/ovsdb-server-2.pid])
>  OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>
>  AT_CLEANUP
> --
> 2.10.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list