[ovs-dev] [PATCH ovn v9 2/8] ovn-controller: I-P for SB port binding and OVS interface in runtime_data.

Numan Siddique numans at ovn.org
Wed Jun 3 07:42:21 UTC 2020


On Wed, Jun 3, 2020 at 10:49 AM Han Zhou <hzhou at ovn.org> wrote:

> On Thu, May 28, 2020 at 4:05 AM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > This patch handles SB port binding changes and OVS interface changes
> > incrementally in the runtime_data stage which otherwise would have
> > resulted in calls to binding_run().
> >
> > Prior to this patch, port binding changes were handled differently.
> > The changes were only evaluated to see if they need full recompute
> > or they can be ignored.
> >
> > With this patch, the runtime data is updated with the changes (both
> > SB port binding and OVS interface) and ports are claimed/released in
> > the handlers itself, avoiding the need to trigger recompute of runtime
> > data stage.
> >
> > Acked-by: Mark Michelson <mmichels at redhat.com>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >  controller/binding.c        | 1002 ++++++++++++++++++++++++++++++-----
> >  controller/binding.h        |    9 +-
> >  controller/ovn-controller.c |   61 ++-
> >  tests/ovn-performance.at    |   13 +
> >  4 files changed, 953 insertions(+), 132 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 6a13d1a0e..74e0e0710 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -360,16 +360,6 @@ destroy_qos_map(struct hmap *qos_map)
> >      hmap_destroy(qos_map);
> >  }
> >
> > -static void
> > -update_local_lport_ids(struct sset *local_lport_ids,
> > -                       const struct sbrec_port_binding *pb)
> > -{
> > -    char buf[16];
> > -    snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> > -             pb->datapath->tunnel_key, pb->tunnel_key);
> > -    sset_add(local_lport_ids, buf);
> > -}
> > -
> >  /*
> >   * Get the encap from the chassis for this port. The interface
> >   * may have an external_ids:encap-ip=<encap-ip> set; if so we
> > @@ -448,10 +438,10 @@ is_network_plugged(const struct sbrec_port_binding
> *binding_rec,
> >  }
> >
> >  static void
> > -consider_localnet_port(const struct sbrec_port_binding *binding_rec,
> > -                       struct shash *bridge_mappings,
> > -                       struct sset *egress_ifaces,
> > -                       struct hmap *local_datapaths)
> > +update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
> > +                        struct shash *bridge_mappings,
> > +                        struct sset *egress_ifaces,
> > +                        struct hmap *local_datapaths)
> >  {
> >      /* Ignore localnet ports for unplugged networks. */
> >      if (!is_network_plugged(binding_rec, bridge_mappings)) {
> > @@ -481,6 +471,27 @@ consider_localnet_port(const struct
> sbrec_port_binding *binding_rec,
> >      ld->localnet_port = binding_rec;
> >  }
> >
> > +static void
> > +update_local_lport_ids(struct sset *local_lport_ids,
> > +                       const struct sbrec_port_binding *pb)
> > +{
> > +    char buf[16];
> > +    snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> > +             pb->datapath->tunnel_key, pb->tunnel_key);
> > +    sset_add(local_lport_ids, buf);
> > +}
> > +
> > +static void
> > +remove_local_lport_ids(const struct sbrec_port_binding *binding_rec,
> > +                       struct sset *local_lport_ids)
> > +{
> > +    char buf[16];
> > +    snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> > +                binding_rec->datapath->tunnel_key,
> > +                binding_rec->tunnel_key);
> > +    sset_find_and_delete(local_lport_ids, buf);
> > +}
> > +
> >  /* Local bindings. binding.c module binds the logical port (represented
> by
> >   * Port_Binding rows) and sets the 'chassis' column when it sees the
> >   * OVS interface row (of type "" or "internal") with the
> > @@ -520,6 +531,10 @@ consider_localnet_port(const struct
> sbrec_port_binding *binding_rec,
> >   *                  BT_VIF. Its Port_Binding row's 'parent' column is
> set to
> >   *                  its parent's Port_Binding. It shares the OVS
> interface row
> >   *                  with the parent.
> > + *                  Each ovn-controller when it sees a container
> Port_Binding,
> > + *                  it creates 'struct local_binding' for the parent
> > + *                  Port_Binding and for its even if the OVS interface
> row for
> > + *                  the parent is not present.
> >   *
> >   *  BT_VIRTUAL: Represents a local binding which has a parent of type
> BT_VIF.
> >   *              Its Port_Binding type is "virtual" and it shares the OVS
> > @@ -527,6 +542,17 @@ consider_localnet_port(const struct
> sbrec_port_binding *binding_rec,
> >   *              Port_Binding of type "virtual" is claimed by pinctrl
> module
> >   *              when it sees the ARP packet from the parent's VIF.
> >   *
> > + *
> > + *  An object of 'struct local_binding' is created:
> > + *    - For each interface that has iface-id configured with the type -
> BT_VIF.
> > + *
> > + *    - For each container Port Binding (of type BT_CONTAINER) and its
> > + *      parent Port_Binding (of type BT_VIF), no matter if
> > + *      they are bound to this chassis i.e even if OVS interface row for
> the
> > + *      parent is not present.
> > + *
> > + *   - For each 'virtual' Port Binding (of type BT_VIRTUAL) provided its
> parent
> > + *     is bound to this chassis.
>
> Numan, thanks for more detailed comments! Still one question here. Why
> would the handling of 'virtual' ports be different from 'container' ports?
> Both of them have parents and if parents are bound to a chassis, the
> children should be considered. Now we create local_binding for all
> container ports, but create local_bindings for virtual ports only if its
> parent is bound to this chassis. Could you explain why this difference?
>

Thanks for the review.

In the case of virtual ports, the port is bound by the pinctrl module when
it sees an ARP
packet from the parent port. So  we don't have to create lbindings for the
parent port
and the virtual port if there is no VIF interface for it.

Let me know if I was not clear enough. I'll add the comment about the same
in v10 p2.

Thanks
Numan


> >   */
> >  enum local_binding_type {
> >      BT_VIF,
> > @@ -598,6 +624,14 @@ local_bindings_destroy(struct shash *local_bindings)
> >      shash_destroy(local_bindings);
> >  }
> >
> > +static
> > +void local_binding_delete(struct shash *local_bindings,
> > +                          struct local_binding *lbinding)
> > +{
> > +    shash_find_and_delete(local_bindings, lbinding->name);
> > +    local_binding_destroy(lbinding);
> > +}
> > +
> >  static void
> >  local_binding_add_child(struct local_binding *lbinding,
> >                          struct local_binding *child)
> > @@ -624,6 +658,7 @@ is_lport_container(const struct sbrec_port_binding
> *pb)
> >      return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0];
> >  }
> >
> > +/* Corresponds to each Port_Binding.type. */
> >  enum en_lport_type {
> >      LP_UNKNOWN,
> >      LP_VIF,
> > @@ -669,12 +704,20 @@ get_lport_type(const struct sbrec_port_binding *pb)
> >      return LP_UNKNOWN;
> >  }
> >
> > -static void
> > +/* Returns false if lport is not claimed due to 'sb_readonly'.
> > + * Returns true otherwise.
> > + */
> > +static bool
> >  claim_lport(const struct sbrec_port_binding *pb,
> >              const struct sbrec_chassis *chassis_rec,
> > -            const struct ovsrec_interface *iface_rec)
> > +            const struct ovsrec_interface *iface_rec,
> > +            bool sb_readonly)
> >  {
> >      if (pb->chassis != chassis_rec) {
> > +        if (sb_readonly) {
> > +            return false;
> > +        }
> > +
> >          if (pb->chassis) {
> >              VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> >                      pb->logical_port, pb->chassis->name,
> > @@ -693,22 +736,44 @@ claim_lport(const struct sbrec_port_binding *pb,
> >      struct sbrec_encap *encap_rec =
> >          sbrec_get_port_encap(chassis_rec, iface_rec);
> >      if (encap_rec && pb->encap != encap_rec) {
> > +        if (sb_readonly) {
> > +            return false;
> > +        }
> >          sbrec_port_binding_set_encap(pb, encap_rec);
> >      }
> > +
> > +    return true;
> >  }
> >
> > -static void
> > -release_lport(const struct sbrec_port_binding *pb)
> > +/* Returns false if lport is not released due to 'sb_readonly'.
> > + * Returns true otherwise.
> > + */
> > +static bool
> > +release_lport(const struct sbrec_port_binding *pb, bool sb_readonly)
> >  {
> > -    VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> >      if (pb->encap) {
> > +        if (sb_readonly) {
> > +            return false;
> > +        }
> >          sbrec_port_binding_set_encap(pb, NULL);
> >      }
> > -    sbrec_port_binding_set_chassis(pb, NULL);
> > +
> > +    if (pb->chassis) {
> > +        if (sb_readonly) {
> > +            return false;
> > +        }
> > +        sbrec_port_binding_set_chassis(pb, NULL);
> > +    }
> >
> >      if (pb->virtual_parent) {
> > +        if (sb_readonly) {
> > +            return false;
> > +        }
> >          sbrec_port_binding_set_virtual_parent(pb, NULL);
> >      }
> > +
> > +    VLOG_INFO("Releasing lport %s from this chassis.",
> pb->logical_port);
> > +    return true;
> >  }
> >
> >  static bool
> > @@ -733,7 +798,63 @@ can_bind_on_this_chassis(const struct sbrec_chassis
> *chassis_rec,
> >             || !strcmp(requested_chassis, chassis_rec->hostname);
> >  }
> >
> > -static void
> > +/* Returns 'true' if the 'lbinding' has children of type BT_CONTAINER,
> > + * 'false' otherwise. */
> > +static bool
> > +is_lbinding_container_parent(struct local_binding *lbinding)
> > +{
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH (node, &lbinding->children) {
> > +        struct local_binding *l = node->data;
> > +        if (l->type == BT_CONTAINER) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +static bool
> > +release_local_binding_children(const struct sbrec_chassis *chassis_rec,
> > +                               struct local_binding *lbinding,
> > +                               bool sb_readonly)
> > +{
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH (node, &lbinding->children) {
> > +        struct local_binding *l = node->data;
> > +        if (is_lbinding_this_chassis(l, chassis_rec)) {
> > +            if (!release_lport(l->pb, sb_readonly)) {
> > +                return false;
> > +            }
> > +        }
> > +
> > +        /* Clear the local bindings' 'pb' and 'iface'. */
> > +        l->pb = NULL;
> > +        l->iface = NULL;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static bool
> > +release_local_binding(const struct sbrec_chassis *chassis_rec,
> > +                      struct local_binding *lbinding, bool sb_readonly)
> > +{
> > +    if (!release_local_binding_children(chassis_rec, lbinding,
> > +                                        sb_readonly)) {
> > +        return false;
> > +    }
> > +
> > +    if (is_lbinding_this_chassis(lbinding, chassis_rec)) {
> > +        return release_lport(lbinding->pb, sb_readonly);
> > +    }
> > +
> > +    lbinding->pb = NULL;
> > +    lbinding->iface = NULL;
> > +    return true;
> > +}
> > +
> > +static bool
> >  consider_vif_lport_(const struct sbrec_port_binding *pb,
> >                      bool can_bind, const char *vif_chassis,
> >                      struct binding_ctx_in *b_ctx_in,
> > @@ -745,7 +866,10 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
> >      if (lbinding_set) {
> >          if (can_bind) {
> >              /* We can claim the lport. */
> > -            claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface);
> > +            if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface,
> > +                             !b_ctx_in->ovnsb_idl_txn)){
> > +                return false;
> > +            }
> >
> >              add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> >                                 b_ctx_in->sbrec_port_binding_by_datapath,
> > @@ -771,13 +895,14 @@ consider_vif_lport_(const struct sbrec_port_binding
> *pb,
> >      if (pb->chassis == b_ctx_in->chassis_rec) {
> >          /* Release the lport if there is no lbinding. */
> >          if (!lbinding_set || !can_bind) {
> > -            release_lport(pb);
> > +            return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
> >          }
> >      }
> >
> > +    return true;
> >  }
> >
> > -static void
> > +static bool
> >  consider_vif_lport(const struct sbrec_port_binding *pb,
> >                     struct binding_ctx_in *b_ctx_in,
> >                     struct binding_ctx_out *b_ctx_out,
> > @@ -797,11 +922,11 @@ consider_vif_lport(const struct sbrec_port_binding
> *pb,
> >          lbinding->pb = pb;
> >      }
> >
> > -    consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> > -                        b_ctx_out, lbinding, qos_map);
> > +    return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> > +                               b_ctx_out, lbinding, qos_map);
> >  }
> >
> > -static void
> > +static bool
> >  consider_container_lport(const struct sbrec_port_binding *pb,
> >                           struct binding_ctx_in *b_ctx_in,
> >                           struct binding_ctx_out *b_ctx_out,
> > @@ -811,7 +936,39 @@ consider_container_lport(const struct
> sbrec_port_binding *pb,
> >      parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> >                                           pb->parent_port);
> >
> > -    if (parent_lbinding && !parent_lbinding->pb) {
> > +    if (!parent_lbinding) {
> > +        /* There is no local_binding for parent port. Create it
> > +         * without OVS interface row. This is the only exception
> > +         * for creating the 'struct local_binding' object without
> > +         * corresponding OVS interface row.
> > +         *
> > +         * This is required for the following reasons:
> > +         *   - If a logical port P1 is created and then
> > +         *     few container ports - C1, C2, .. are created first by
> CMS.
> > +         *   - And later when OVS interface row  is created for P1, then
> > +         *     we want the these container ports also be claimed by the
> > +         *     chassis.
> > +         * */
> > +        parent_lbinding = local_binding_create(pb->parent_port, NULL,
> NULL,
> > +                                               BT_VIF);
> > +        local_binding_add(b_ctx_out->local_bindings, parent_lbinding);
> > +    }
> > +
> > +    struct local_binding *container_lbinding =
> > +        local_binding_find_child(parent_lbinding, pb->logical_port);
> > +
> > +    if (!container_lbinding) {
> > +        container_lbinding = local_binding_create(pb->logical_port,
> > +
> parent_lbinding->iface,
> > +                                                  pb, BT_CONTAINER);
> > +        local_binding_add_child(parent_lbinding, container_lbinding);
> > +    } else {
> > +        ovs_assert(container_lbinding->type == BT_CONTAINER);
> > +        container_lbinding->pb = pb;
> > +        container_lbinding->iface = parent_lbinding->iface;
> > +    }
> > +
> > +    if (!parent_lbinding->pb) {
> >          parent_lbinding->pb = lport_lookup_by_name(
> >              b_ctx_in->sbrec_port_binding_by_name, pb->parent_port);
> >
> > @@ -820,37 +977,28 @@ consider_container_lport(const struct
> sbrec_port_binding *pb,
> >               * So call consider_vif_lport() to process it first. */
> >              consider_vif_lport(parent_lbinding->pb, b_ctx_in, b_ctx_out,
> >                                 parent_lbinding, qos_map);
> > -        }
> > -    }
> > +        } else {
> > +            /* The parent lport doesn't exist. Call release_lport() to
> > +             * release the container lport, if it was bound earlier. */
> > +            if (is_lbinding_this_chassis(container_lbinding,
> > +                                         b_ctx_in->chassis_rec)) {
> > +               return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
> > +            }
> >
> > -    if (!parent_lbinding || !parent_lbinding->pb) {
> > -        /* Call release_lport, to release the container lport, if
> > -         * it was bound earlier. */
> > -        if (pb->chassis == b_ctx_in->chassis_rec) {
> > -            release_lport(pb);
> > +            return true;
> >          }
> > -        return;
> >      }
> >
> > -    struct local_binding *container_lbinding =
> > -        local_binding_find_child(parent_lbinding, pb->logical_port);
> > -    ovs_assert(!container_lbinding);
> > -
> > -    container_lbinding = local_binding_create(pb->logical_port,
> > -                                              parent_lbinding->iface,
> > -                                              pb, BT_CONTAINER);
> > -    local_binding_add_child(parent_lbinding, container_lbinding);
> > -
> >      const char *vif_chassis = smap_get(&parent_lbinding->pb->options,
> >                                         "requested-chassis");
> >      bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec,
> >                                               vif_chassis);
> >
> > -    consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, b_ctx_out,
> > -                        container_lbinding, qos_map);
> > +    return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> b_ctx_out,
> > +                               container_lbinding, qos_map);
> >  }
> >
> > -static void
> > +static bool
> >  consider_virtual_lport(const struct sbrec_port_binding *pb,
> >                         struct binding_ctx_in *b_ctx_in,
> >                         struct binding_ctx_out *b_ctx_out,
> > @@ -877,11 +1025,16 @@ consider_virtual_lport(const struct
> sbrec_port_binding *pb,
> >      if (is_lbinding_this_chassis(parent_lbinding,
> b_ctx_in->chassis_rec)) {
> >          virtual_lbinding =
> >              local_binding_find_child(parent_lbinding, pb->logical_port);
> > -        ovs_assert(!virtual_lbinding);
> > -        virtual_lbinding = local_binding_create(pb->logical_port,
> > -                                                parent_lbinding->iface,
> > -                                                pb, BT_VIRTUAL);
> > -        local_binding_add_child(parent_lbinding, virtual_lbinding);
> > +        if (!virtual_lbinding) {
> > +            virtual_lbinding = local_binding_create(pb->logical_port,
> > +
>  parent_lbinding->iface,
> > +                                                    pb, BT_VIRTUAL);
> > +            local_binding_add_child(parent_lbinding, virtual_lbinding);
> > +        } else {
> > +            ovs_assert(virtual_lbinding->type == BT_VIRTUAL);
> > +            virtual_lbinding->pb = pb;
> > +            virtual_lbinding->iface = parent_lbinding->iface;
> > +        }
> >      }
> >
> >      return consider_vif_lport_(pb, true, NULL, b_ctx_in, b_ctx_out,
> > @@ -891,14 +1044,13 @@ consider_virtual_lport(const struct
> sbrec_port_binding *pb,
> >  /* Considers either claiming the lport or releasing the lport
> >   * for non VIF lports.
> >   */
> > -static void
> > +static bool
> >  consider_nonvif_lport_(const struct sbrec_port_binding *pb,
> >                         bool our_chassis,
> >                         bool has_local_l3gateway,
> >                         struct binding_ctx_in *b_ctx_in,
> >                         struct binding_ctx_out *b_ctx_out)
> >  {
> > -    ovs_assert(b_ctx_in->ovnsb_idl_txn);
> >      if (our_chassis) {
> >          sset_add(b_ctx_out->local_lports, pb->logical_port);
> >          add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> > @@ -908,13 +1060,16 @@ consider_nonvif_lport_(const struct
> sbrec_port_binding *pb,
> >                             b_ctx_out->local_datapaths);
> >
> >          update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > -        claim_lport(pb, b_ctx_in->chassis_rec, NULL);
> > +        return claim_lport(pb, b_ctx_in->chassis_rec, NULL,
> > +                           !b_ctx_in->ovnsb_idl_txn);
> >      } else if (pb->chassis == b_ctx_in->chassis_rec) {
> > -        release_lport(pb);
> > +        return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
> >      }
> > +
> > +    return true;
> >  }
> >
> > -static void
> > +static bool
> >  consider_l2gw_lport(const struct sbrec_port_binding *pb,
> >                      struct binding_ctx_in *b_ctx_in,
> >                      struct binding_ctx_out *b_ctx_out)
> > @@ -923,10 +1078,10 @@ consider_l2gw_lport(const struct
> sbrec_port_binding *pb,
> >      bool our_chassis = chassis_id && !strcmp(chassis_id,
> >
> b_ctx_in->chassis_rec->name);
> >
> > -    consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
> > +    return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in,
> b_ctx_out);
> >  }
> >
> > -static void
> > +static bool
> >  consider_l3gw_lport(const struct sbrec_port_binding *pb,
> >                      struct binding_ctx_in *b_ctx_in,
> >                      struct binding_ctx_out *b_ctx_out)
> > @@ -935,7 +1090,7 @@ consider_l3gw_lport(const struct sbrec_port_binding
> *pb,
> >      bool our_chassis = chassis_id && !strcmp(chassis_id,
> >
> b_ctx_in->chassis_rec->name);
> >
> > -    consider_nonvif_lport_(pb, our_chassis, true, b_ctx_in, b_ctx_out);
> > +    return consider_nonvif_lport_(pb, our_chassis, true, b_ctx_in,
> b_ctx_out);
> >  }
> >
> >  static void
> > @@ -954,7 +1109,7 @@ consider_localnet_lport(const struct
> sbrec_port_binding *pb,
> >      update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> >  }
> >
> > -static void
> > +static bool
> >  consider_ha_lport(const struct sbrec_port_binding *pb,
> >                    struct binding_ctx_in *b_ctx_in,
> >                    struct binding_ctx_out *b_ctx_out)
> > @@ -982,18 +1137,18 @@ consider_ha_lport(const struct sbrec_port_binding
> *pb,
> >                             b_ctx_out->local_datapaths);
> >      }
> >
> > -    consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, b_ctx_out);
> > +    return consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in,
> b_ctx_out);
> >  }
> >
> > -static void
> > +static bool
> >  consider_cr_lport(const struct sbrec_port_binding *pb,
> >                    struct binding_ctx_in *b_ctx_in,
> >                    struct binding_ctx_out *b_ctx_out)
> >  {
> > -    consider_ha_lport(pb, b_ctx_in, b_ctx_out);
> > +    return consider_ha_lport(pb, b_ctx_in, b_ctx_out);
> >  }
> >
> > -static void
> > +static bool
> >  consider_external_lport(const struct sbrec_port_binding *pb,
> >                          struct binding_ctx_in *b_ctx_in,
> >                          struct binding_ctx_out *b_ctx_out)
> > @@ -1046,6 +1201,8 @@ build_local_bindings(struct binding_ctx_in
> *b_ctx_in,
> >                  }
> >
> >                  sset_add(b_ctx_out->local_lports, iface_id);
> > +                smap_replace(b_ctx_out->local_iface_ids,
> iface_rec->name,
> > +                             iface_id);
> >              }
> >
> >              /* Check if this is a tunnel interface. */
> > @@ -1161,9 +1318,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
> binding_ctx_out *b_ctx_out)
> >       * corresponding local datapath accordingly. */
> >      struct localnet_lport *lnet_lport;
> >      LIST_FOR_EACH_POP (lnet_lport, list_node, &localnet_lports) {
> > -        consider_localnet_port(lnet_lport->pb, &bridge_mappings,
> > -                               b_ctx_out->egress_ifaces,
> > -                               b_ctx_out->local_datapaths);
> > +        update_ld_localnet_port(lnet_lport->pb, &bridge_mappings,
> > +                                b_ctx_out->egress_ifaces,
> > +                                b_ctx_out->local_datapaths);
> >          free(lnet_lport);
> >      }
> >
> > @@ -1181,95 +1338,688 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> struct binding_ctx_out *b_ctx_out)
> >      destroy_qos_map(&qos_map);
> >  }
> >
> > -/* Returns true if port-binding changes potentially require flow changes
> on
> > - * the current chassis. Returns false if we are sure there is no impact.
> */
> > +/* Returns true if the database is all cleaned up, false if more work is
> > + * required. */
> >  bool
> > -binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in,
> > -                                      struct binding_ctx_out *b_ctx_out)
> > +binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > +                const struct sbrec_port_binding_table
> *port_binding_table,
> > +                const struct sbrec_chassis *chassis_rec)
> >  {
> > -    if (!b_ctx_in->chassis_rec) {
> > +    if (!ovnsb_idl_txn) {
> > +        return false;
> > +    }
> > +    if (!chassis_rec) {
> >          return true;
> >      }
> >
> > -    bool changed = false;
> > -
> >      const struct sbrec_port_binding *binding_rec;
> > -    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec,
> > -
> b_ctx_in->port_binding_table) {
> > -        /* XXX: currently OVSDB change tracking doesn't support getting
> old
> > -         * data when the operation is update, so if a port-binding moved
> from
> > -         * this chassis to another, there is no easy way to find out the
> > -         * change. To workaround this problem, we just makes sure if
> > -         * any port *related to* this chassis has any change, then
> trigger
> > -         * recompute.
> > -         *
> > -         * - If a regular VIF is unbound from this chassis, the local
> ovsdb
> > -         *   interface table will be updated, which will trigger
> recompute.
> > -         *
> > -         * - If the port is not a regular VIF, always trigger recompute.
> */
> > -        if (binding_rec->chassis == b_ctx_in->chassis_rec) {
> > -            changed = true;
> > +    bool any_changes = false;
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table)
> {
> > +        if (binding_rec->chassis == chassis_rec) {
> > +            if (binding_rec->encap) {
> > +                sbrec_port_binding_set_encap(binding_rec, NULL);
> > +            }
> > +            sbrec_port_binding_set_chassis(binding_rec, NULL);
> > +            any_changes = true;
> > +        }
> > +    }
> > +
> > +    if (any_changes) {
> > +        ovsdb_idl_txn_add_comment(
> > +            ovnsb_idl_txn,
> > +            "ovn-controller: removing all port bindings for '%s'",
> > +            chassis_rec->name);
> > +    }
> > +
> > +    return !any_changes;
> > +}
> > +
> > +/* This function adds the local datapath of the 'peer' of
> > + * lport 'pb' to the local datapaths if it is not yet added.
> > + */
> > +static void
> > +add_local_datapath_peer_port(const struct sbrec_port_binding *pb,
> > +                             struct binding_ctx_in *b_ctx_in,
> > +                             struct binding_ctx_out *b_ctx_out,
> > +                             struct local_datapath *ld)
> > +{
> > +    const char *peer_name = smap_get(&pb->options, "peer");
> > +    if (strcmp(pb->type, "patch") || !peer_name) {
> > +        return;
> > +    }
> > +
> > +    const struct sbrec_port_binding *peer;
> > +    peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> > +                                peer_name);
> > +
> > +    if (!peer || !peer->datapath) {
> > +        return;
> > +    }
> > +
> > +    bool present = false;
> > +    for (size_t i = 0; i < ld->n_peer_ports; i++) {
> > +        if (ld->peer_ports[i].local == pb) {
> > +            present = true;
> >              break;
> >          }
> > +    }
> >
> > -        if (!strcmp(binding_rec->type, "remote")) {
> > -            continue;
> > +    if (!present) {
> > +        ld->n_peer_ports++;
> > +        if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> > +            ld->peer_ports =
> > +                x2nrealloc(ld->peer_ports,
> > +                           &ld->n_allocated_peer_ports,
> > +                           sizeof *ld->peer_ports);
> >          }
> > +        ld->peer_ports[ld->n_peer_ports - 1].local = pb;
> > +        ld->peer_ports[ld->n_peer_ports - 1].remote = peer;
> > +    }
> > +
> > +    struct local_datapath *peer_ld =
> > +        get_local_datapath(b_ctx_out->local_datapaths,
> > +                           peer->datapath->tunnel_key);
> > +    if (!peer_ld) {
> > +        add_local_datapath__(b_ctx_in->sbrec_datapath_binding_by_key,
> > +                             b_ctx_in->sbrec_port_binding_by_datapath,
> > +                             b_ctx_in->sbrec_port_binding_by_name,
> > +                             peer->datapath, false,
> > +                             1, b_ctx_out->local_datapaths);
> > +        return;
> > +    }
> >
> > -        if (strcmp(binding_rec->type, "")) {
> > -            changed = true;
> > +    for (size_t i = 0; i < peer_ld->n_peer_ports; i++) {
> > +        if (peer_ld->peer_ports[i].local == peer) {
> > +            return;
> > +        }
> > +    }
> > +
> > +    peer_ld->n_peer_ports++;
> > +    if (peer_ld->n_peer_ports > peer_ld->n_allocated_peer_ports) {
> > +        peer_ld->peer_ports =
> > +            x2nrealloc(peer_ld->peer_ports,
> > +                        &peer_ld->n_allocated_peer_ports,
> > +                        sizeof *peer_ld->peer_ports);
> > +    }
> > +    peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = peer;
> > +    peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb;
> > +}
> > +
> > +static void
> > +remove_local_datapath_peer_port(const struct sbrec_port_binding *pb,
> > +                                struct local_datapath *ld,
> > +                                struct hmap *local_datapaths)
> > +{
> > +    size_t i = 0;
> > +    for (i = 0; i < ld->n_peer_ports; i++) {
> > +        if (ld->peer_ports[i].local == pb) {
> >              break;
> >          }
> > +    }
> > +
> > +    if (i == ld->n_peer_ports) {
> > +        return;
> > +    }
> > +
> > +    const struct sbrec_port_binding *peer = ld->peer_ports[i].remote;
> >
> > -        struct local_binding *lbinding = NULL;
> > -        if (!binding_rec->parent_port || !binding_rec->parent_port[0]) {
> > -            lbinding = local_binding_find(b_ctx_out->local_bindings,
> > -                                          binding_rec->logical_port);
> > +    /* Possible improvement: We can shrint the allocated peer ports
> > +     * if (ld->n_peer_ports < ld->n_allocated_peer_ports / 2).
> > +     */
> > +    ld->peer_ports[i].local = ld->peer_ports[ld->n_peer_ports -
> 1].local;
> > +    ld->peer_ports[i].remote = ld->peer_ports[ld->n_peer_ports -
> 1].remote;
> > +    ld->n_peer_ports--;
> > +
> > +    struct local_datapath *peer_ld =
> > +        get_local_datapath(local_datapaths, peer->datapath->tunnel_key);
> > +    if (peer_ld) {
> > +        /* Remove the peer port from the peer datapath. The peer
> > +         * datapath also tries to remove its peer lport, but that would
> > +         * be no-op. */
> > +        remove_local_datapath_peer_port(peer, peer_ld, local_datapaths);
> > +    }
> > +}
> > +
> > +static void
> > +remove_pb_from_local_datapath(const struct sbrec_port_binding *pb,
> > +                              const struct sbrec_chassis *chassis_rec,
> > +                              struct binding_ctx_out *b_ctx_out,
> > +                              struct local_datapath *ld)
> > +{
> > +    remove_local_lport_ids(pb, b_ctx_out->local_lport_ids);
> > +    if (!strcmp(pb->type, "patch") ||
> > +        !strcmp(pb->type, "l3gateway")) {
> > +        remove_local_datapath_peer_port(pb, ld,
> b_ctx_out->local_datapaths);
> > +    } else if (!strcmp(pb->type, "localnet")) {
> > +        if (ld->localnet_port &&
> !strcmp(ld->localnet_port->logical_port,
> > +                                         pb->logical_port)) {
> > +            ld->localnet_port = NULL;
> > +        }
> > +    } else if (!strcmp(pb->type, "l3gateway")) {
> > +        const char *chassis_id = smap_get(&pb->options,
> > +                                          "l3gateway-chassis");
> > +        if (chassis_id && !strcmp(chassis_id, chassis_rec->name)) {
> > +            ld->has_local_l3gateway = false;
> > +        }
> > +    }
> > +}
> > +
> > +/* Considers the ovs iface 'iface_rec' for claiming.
> > + * This function should be called if the external_ids:iface-id
> > + * and 'ofport' are set for the 'iface_rec'.
> > + *
> > + * If the local_binding for this 'iface_rec' already exists and its
> > + * already claimed, then this function will be no-op.
> > + */
> > +static bool
> > +consider_iface_claim(const struct ovsrec_interface *iface_rec,
> > +                     const char *iface_id,
> > +                     struct binding_ctx_in *b_ctx_in,
> > +                     struct binding_ctx_out *b_ctx_out,
> > +                     struct hmap *qos_map)
> > +{
> > +    sset_add(b_ctx_out->local_lports, iface_id);
> > +    smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, iface_id);
> > +
> > +    struct local_binding *lbinding =
> > +        local_binding_find(b_ctx_out->local_bindings, iface_id);
> > +
> > +    if (!lbinding) {
> > +        lbinding = local_binding_create(iface_id, iface_rec, NULL,
> BT_VIF);
> > +        local_binding_add(b_ctx_out->local_bindings, lbinding);
> > +    } else {
> > +        lbinding->iface = iface_rec;
> > +    }
> > +
> > +    if (!lbinding->pb || strcmp(lbinding->name,
> lbinding->pb->logical_port)) {
> > +        lbinding->pb = lport_lookup_by_name(
> > +            b_ctx_in->sbrec_port_binding_by_name, lbinding->name);
> > +        if (lbinding->pb && !strcmp(lbinding->pb->type, "virtual")) {
> > +            lbinding->pb = NULL;
> > +        }
> > +    }
> > +
> > +    if (lbinding->pb) {
> > +        if (!consider_vif_lport(lbinding->pb, b_ctx_in, b_ctx_out,
> > +                                lbinding, qos_map)) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    /* Update the child local_binding's iface (if any children) and try
> to
> > +     *  claim the container lbindings. */
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH (node, &lbinding->children) {
> > +        struct local_binding *child = node->data;
> > +        child->iface = iface_rec;
> > +        if (child->type == BT_CONTAINER) {
> > +            if (!consider_container_lport(child->pb, b_ctx_in,
> b_ctx_out,
> > +                                          qos_map)) {
> > +                return false;
> > +            }
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +/* Considers the ovs interface 'iface_rec' for
> > + * releasing from this chassis if local_binding for this
> > + * 'iface_rec' (with 'iface_id' as key) already exists and
> > + * it is claimed by the chassis.
> > + *
> > + * The 'iface_id' could be cleared from the 'iface_rec'
> > + * and hence it is passed separately.
> > + *
> > + * This fuction should be called if
> > + *   - OVS interface 'iface_rec' is deleted.
> > + *   - OVS interface 'iface_rec' external_ids:iface-id is updated
> > + *     (with the old value being 'iface_id'.)
> > + *   - OVS interface ofport is reset to 0.
> > + * */
> > +static bool
> > +consider_iface_release(const struct ovsrec_interface *iface_rec,
> > +                       const char *iface_id,
> > +                       struct binding_ctx_in *b_ctx_in,
> > +                       struct binding_ctx_out *b_ctx_out, bool *changed)
> > +{
> > +    struct local_binding *lbinding;
> > +    lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                  iface_id);
> > +    if (is_lbinding_this_chassis(lbinding, b_ctx_in->chassis_rec)) {
> > +
> > +        if (!release_local_binding(b_ctx_in->chassis_rec, lbinding,
> > +                                   !b_ctx_in->ovnsb_idl_txn)) {
> > +            return false;
> > +        }
> > +
> > +        struct local_datapath *ld =
> > +            get_local_datapath(b_ctx_out->local_datapaths,
> > +                                lbinding->pb->datapath->tunnel_key);
> > +        if (ld) {
> > +            remove_pb_from_local_datapath(lbinding->pb,
> > +                                            b_ctx_in->chassis_rec,
> > +                                            b_ctx_out, ld);
> > +        }
> > +
> > +        /* Check if the lbinding has children of type PB_CONTAINER.
> > +         * If so, don't delete the local_binding. */
> > +        if (!is_lbinding_container_parent(lbinding)) {
> > +            local_binding_delete(b_ctx_out->local_bindings, lbinding);
> > +        }
> > +        *changed = true;
> > +    }
> > +
> > +    sset_find_and_delete(b_ctx_out->local_lports, iface_id);
> > +    smap_remove(b_ctx_out->local_iface_ids, iface_rec->name);
> > +
> > +    return true;
> > +}
> > +
> > +static bool
> > +is_iface_vif(const struct ovsrec_interface *iface_rec)
> > +{
> > +    if (iface_rec->type && iface_rec->type[0] &&
> > +        strcmp(iface_rec->type, "internal")) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +/* Returns true if the ovs interface changes were handled successfully,
> > + * false otherwise.
> > + */
> > +bool
> > +binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in,
> > +                                     struct binding_ctx_out *b_ctx_out,
> > +                                     bool *changed)
> > +{
> > +    if (!b_ctx_in->chassis_rec) {
> > +        return false;
> > +    }
> > +
> > +    bool handled = true;
> > +    *changed = false;
> > +
> > +    /* Run the tracked interfaces loop twice. One to handle deleted
> > +     * changes. And another to handle add/update changes.
> > +     * This will ensure correctness.
> > +     *     *
> > +     * We consider an OVS interface for release if one of the following
> > +     * happens:
> > +     *   1. OVS interface is deleted.
> > +     *   2. external_ids:iface-id is cleared in which case we need to
> > +     *      release the port binding corresponding to the previously set
> > +     *      'old-iface-id' (which is stored in the smap
> > +     *      'b_ctx_out->local_iface_ids').
> > +     *   3. external_ids:iface-id is updated with a different value
> > +     *      in which case we need to release the port binding
> corresponding
> > +     *      to the previously set 'old-iface-id' (which is stored in the
> smap
> > +     *      'b_ctx_out->local_iface_ids').
> > +     *   4. ofport of the OVS interface is 0.
> > +     *
> > +     */
> > +    const struct ovsrec_interface *iface_rec;
> > +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> > +                                             b_ctx_in->iface_table) {
> > +        if (!is_iface_vif(iface_rec)) {
> > +            /* Right now are not handling ovs_interface changes of
> > +             * other types. This can be enhanced to handle of
> > +             * types - patch and tunnel. */
> > +            handled = false;
> > +            break;
> > +        }
> > +
> > +        const char *iface_id = smap_get(&iface_rec->external_ids,
> "iface-id");
> > +        const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids,
> > +                                            iface_rec->name);
> > +        const char *cleared_iface_id = NULL;
> > +        if (!ovsrec_interface_is_deleted(iface_rec)) {
> > +            int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
> 0;
> > +            if (iface_id) {
> > +                /* Check if iface_id is changed. If so we need to
> > +                 * release the old port binding and associate this
> > +                 * inteface to new port binding. */
> > +                if (old_iface_id && strcmp(iface_id, old_iface_id)) {
> > +                    cleared_iface_id = old_iface_id;
> > +                } else if (!ofport) {
> > +                    /* If ofport is 0, we need to release the iface if
> already
> > +                     * claimed. */
> > +                    cleared_iface_id = iface_id;
> > +                }
> > +            } else if (old_iface_id) {
> > +                cleared_iface_id = old_iface_id;
> > +            }
> >          } else {
> > -            lbinding = local_binding_find(b_ctx_out->local_bindings,
> > -                                          binding_rec->parent_port);
> > +            cleared_iface_id = iface_id;
> >          }
> >
> > -        if (lbinding) {
> > -            changed = true;
> > +        if (cleared_iface_id) {
> > +            handled = consider_iface_release(iface_rec,
> cleared_iface_id,
> > +                                             b_ctx_in, b_ctx_out,
> changed);
> > +        }
> > +
> > +        if (!handled) {
> >              break;
> >          }
> >      }
> >
> > -    return changed;
> > +    if (!handled) {
> > +        /* This can happen if any non vif OVS interface is in the
> tracked
> > +         * list or if consider_iface_release() returned false.
> > +         * There is no need to process further. */
> > +        return false;
> > +    }
> > +
> > +    struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > +    struct hmap *qos_map_ptr =
> > +        sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > +
> > +    /*
> > +     * We consider an OVS interface for claiming if the following
> > +     * 2 conditions are met:
> > +     *   1. external_ids:iface-id is set.
> > +     *   2. ofport of the OVS interface is > 0.
> > +     *
> > +     * So when an update of an OVS interface happens we see if these
> > +     * conditions are still true. If so consider this interface for
> > +     * claiming. This would be no-op if the update of the OVS interface
> > +     * didn't change the above two fields.
> > +     */
> > +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> > +                                             b_ctx_in->iface_table) {
> > +        /* Loop to handle create and update changes only. */
> > +        if (ovsrec_interface_is_deleted(iface_rec)) {
> > +            continue;
> > +        }
> > +
> > +        const char *iface_id = smap_get(&iface_rec->external_ids,
> "iface-id");
> > +        int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
> > +        if (iface_id && ofport > 0) {
> > +            *changed = true;
> > +            handled = consider_iface_claim(iface_rec, iface_id,
> b_ctx_in,
> > +                                           b_ctx_out, qos_map_ptr);
> > +            if (!handled) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
> > +                                               b_ctx_in->port_table,
> > +                                               b_ctx_in->qos_table,
> > +
> b_ctx_out->egress_ifaces)) {
> > +        const char *entry;
> > +        SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > +            setup_qos(entry, &qos_map);
> > +        }
> > +    }
> > +
> > +    destroy_qos_map(&qos_map);
> > +    return handled;
> >  }
> >
> > -/* Returns true if the database is all cleaned up, false if more work is
> > - * required. */
> > -bool
> > -binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > -                const struct sbrec_port_binding_table
> *port_binding_table,
> > -                const struct sbrec_chassis *chassis_rec)
> > +static void
> > +handle_deleted_lport(const struct sbrec_port_binding *pb,
> > +                     struct binding_ctx_in *b_ctx_in,
> > +                     struct binding_ctx_out *b_ctx_out)
> >  {
> > -    if (!ovnsb_idl_txn) {
> > +    struct local_datapath *ld =
> > +        get_local_datapath(b_ctx_out->local_datapaths,
> > +                           pb->datapath->tunnel_key);
> > +    if (ld) {
> > +        remove_pb_from_local_datapath(pb, b_ctx_in->chassis_rec,
> > +                                      b_ctx_out, ld);
> > +    }
> > +}
> > +
> > +static struct local_binding *
> > +get_lbinding_for_lport(const struct sbrec_port_binding *pb,
> > +                       enum en_lport_type lport_type,
> > +                       struct binding_ctx_out *b_ctx_out)
> > +{
> > +    ovs_assert(lport_type == LP_VIF || lport_type == LP_VIRTUAL);
> > +
> > +    if (lport_type == LP_VIF && !is_lport_container(pb)) {
> > +        return local_binding_find(b_ctx_out->local_bindings,
> pb->logical_port);
> > +    }
> > +
> > +    struct local_binding *parent_lbinding = NULL;
> > +
> > +    if (lport_type == LP_VIRTUAL) {
> > +        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                             pb->virtual_parent);
> > +    } else {
> > +        parent_lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                             pb->parent_port);
> > +    }
> > +
> > +    return parent_lbinding
> > +           ? local_binding_find(&parent_lbinding->children,
> pb->logical_port)
> > +           : NULL;
> > +}
> > +
> > +static bool
> > +handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
> > +                         enum en_lport_type lport_type,
> > +                         struct binding_ctx_in *b_ctx_in,
> > +                         struct binding_ctx_out *b_ctx_out,
> > +                         bool *changed)
> > +{
> > +    struct local_binding *lbinding =
> > +        get_lbinding_for_lport(pb, lport_type, b_ctx_out);
> > +
> > +    if (lbinding) {
> > +        lbinding->pb = NULL;
> > +        /* The port_binding 'pb' is deleted. So there is no need to
> > +         * clear the 'chassis' column of 'pb'. But we need to do
> > +         * for the local_binding's children. */
> > +        if (lbinding->type == BT_VIF &&
> > +                !release_local_binding_children(b_ctx_in->chassis_rec,
> > +                                                lbinding,
> > +
>  !b_ctx_in->ovnsb_idl_txn)) {
> > +            return false;
> > +        }
> > +
> > +        *changed = true;
> > +    }
> > +
> > +    handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
> > +    return true;
> > +}
> > +
> > +static bool
> > +handle_updated_vif_lport(const struct sbrec_port_binding *pb,
> > +                         enum en_lport_type lport_type,
> > +                         struct binding_ctx_in *b_ctx_in,
> > +                         struct binding_ctx_out *b_ctx_out,
> > +                         struct hmap *qos_map,
> > +                         bool *changed)
> > +{
> > +    bool claimed = (pb->chassis == b_ctx_in->chassis_rec);
> > +    bool handled = true;
> > +
> > +    if (lport_type == LP_VIRTUAL) {
> > +        handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out,
> qos_map);
> > +    } else if (lport_type == LP_VIF && is_lport_container(pb)) {
> > +        handled = consider_container_lport(pb, b_ctx_in, b_ctx_out,
> qos_map);
> > +    } else {
> > +        handled = consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> qos_map);
> > +    }
> > +
> > +    if (!handled) {
> >          return false;
> >      }
> > -    if (!chassis_rec) {
> > +
> > +    bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec);
> > +    bool changed_ = (claimed != now_claimed);
> > +
> > +    if (changed_) {
> > +        *changed = true;
> > +    }
> > +
> > +    if (lport_type == LP_VIRTUAL ||
> > +        (lport_type == LP_VIF && is_lport_container(pb)) || !changed_) {
> >          return true;
> >      }
> >
> > -    const struct sbrec_port_binding *binding_rec;
> > -    bool any_changes = false;
> > -    SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table)
> {
> > -        if (binding_rec->chassis == chassis_rec) {
> > -            if (binding_rec->encap)
> > -                sbrec_port_binding_set_encap(binding_rec, NULL);
> > -            sbrec_port_binding_set_chassis(binding_rec, NULL);
> > -            any_changes = true;
> > +    struct local_binding *lbinding =
> > +        local_binding_find(b_ctx_out->local_bindings, pb->logical_port);
> > +
> > +    ovs_assert(lbinding);
> > +
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH (node, &lbinding->children) {
> > +        struct local_binding *child = node->data;
> > +        if (child->type == BT_CONTAINER) {
> > +            handled = consider_container_lport(child->pb, b_ctx_in,
> b_ctx_out,
> > +                                               qos_map);
> > +            if (!handled) {
> > +                return false;
> > +            }
> >          }
> >      }
> >
> > -    if (any_changes) {
> > -        ovsdb_idl_txn_add_comment(
> > -            ovnsb_idl_txn,
> > -            "ovn-controller: removing all port bindings for '%s'",
> > -            chassis_rec->name);
> > +    return true;
> > +}
> > +
> > +/* Returns true if the port binding changes resulted in local binding
> > + * updates, false otherwise.
> > + */
> > +bool
> > +binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
> > +                                    struct binding_ctx_out *b_ctx_out,
> > +                                    bool *changed)
> > +{
> > +    bool handled = true;
> > +    *changed = false;
> > +
> > +    const struct sbrec_port_binding *pb;
> > +
> > +    /* Run the tracked port binding loop twice. One to handle deleted
> > +     * changes. And another to handle add/update changes.
> > +     * This will ensure correctness.
> > +     */
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > +
> b_ctx_in->port_binding_table) {
> > +        if (!sbrec_port_binding_is_deleted(pb)) {
> > +            continue;
> > +        }
> > +
> > +        enum en_lport_type lport_type = get_lport_type(pb);
> > +        if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) {
> > +            handled = handle_deleted_vif_lport(pb, lport_type, b_ctx_in,
> > +                                                b_ctx_out, changed);
> > +        } else {
> > +            handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
> > +        }
> > +
> > +        if (!handled) {
> > +            break;
> > +        }
> >      }
> >
> > -    return !any_changes;
> > +    if (!handled) {
> > +        return false;
> > +    }
> > +
> > +    struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > +    struct hmap *qos_map_ptr =
> > +        sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > +
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > +
> b_ctx_in->port_binding_table) {
> > +        /* Loop to handle create and update changes only. */
> > +        if (sbrec_port_binding_is_deleted(pb)) {
> > +            continue;
> > +        }
> > +
> > +        enum en_lport_type lport_type = get_lport_type(pb);
> > +
> > +        struct local_datapath *ld =
> > +            get_local_datapath(b_ctx_out->local_datapaths,
> > +                               pb->datapath->tunnel_key);
> > +
> > +        switch (lport_type) {
> > +        case LP_VIF:
> > +        case LP_VIRTUAL:
> > +            handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in,
> > +                                               b_ctx_out, qos_map_ptr,
> > +                                               changed);
> > +            break;
> > +
> > +        case LP_PATCH:
> > +        case LP_LOCALPORT:
> > +        case LP_VTEP:
> > +            *changed = true;
> > +            update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > +            if (lport_type ==  LP_PATCH) {
> > +                /* Add the peer datapath to the local datapaths if it's
> > +                 * not present yet.
> > +                 */
> > +                if (ld) {
> > +                    add_local_datapath_peer_port(pb, b_ctx_in,
> b_ctx_out, ld);
> > +                }
> > +            }
> > +            break;
> > +
> > +        case LP_L2GATEWAY:
> > +            *changed = true;
> > +            handled = consider_l2gw_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_L3GATEWAY:
> > +            *changed = true;
> > +            handled = consider_l3gw_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_CHASSISREDIRECT:
> > +            *changed = true;
> > +            handled = consider_cr_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_EXTERNAL:
> > +            *changed = true;
> > +            handled = consider_external_lport(pb, b_ctx_in, b_ctx_out);
> > +            break;
> > +
> > +        case LP_LOCALNET: {
> > +            *changed = true;
> > +            consider_localnet_lport(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> > +
> > +            struct shash bridge_mappings =
> > +                SHASH_INITIALIZER(&bridge_mappings);
> > +            add_ovs_bridge_mappings(b_ctx_in->ovs_table,
> > +                                    b_ctx_in->bridge_table,
> > +                                    &bridge_mappings);
> > +            update_ld_localnet_port(pb, &bridge_mappings,
> > +                                    b_ctx_out->egress_ifaces,
> > +                                    b_ctx_out->local_datapaths);
> > +            shash_destroy(&bridge_mappings);
> > +            break;
> > +        }
> > +
> > +        case LP_REMOTE:
> > +        case LP_UNKNOWN:
> > +            break;
> > +        }
> > +
> > +        if (!handled) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
> > +                                               b_ctx_in->port_table,
> > +                                               b_ctx_in->qos_table,
> > +
> b_ctx_out->egress_ifaces)) {
> > +        const char *entry;
> > +        SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > +            setup_qos(entry, &qos_map);
> > +        }
> > +    }
> > +
> > +    destroy_qos_map(&qos_map);
> > +    return handled;
> >  }
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 9affc9a96..f7ace6faf 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -57,6 +57,7 @@ struct binding_ctx_out {
> >      struct sset *local_lports;
> >      struct sset *local_lport_ids;
> >      struct sset *egress_ifaces;
> > +    struct smap *local_iface_ids;
> >  };
> >
> >  void binding_register_ovs_idl(struct ovsdb_idl *);
> > @@ -64,9 +65,13 @@ void binding_run(struct binding_ctx_in *, struct
> binding_ctx_out *);
> >  bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >                       const struct sbrec_port_binding_table *,
> >                       const struct sbrec_chassis *);
> > -bool binding_evaluate_port_binding_changes(struct binding_ctx_in *,
> > -                                           struct binding_ctx_out *);
> >
> >  void local_bindings_init(struct shash *local_bindings);
> >  void local_bindings_destroy(struct shash *local_bindings);
> > +bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
> > +                                          struct binding_ctx_out *,
> > +                                          bool *changed);
> > +bool binding_handle_port_binding_changes(struct binding_ctx_in *,
> > +                                         struct binding_ctx_out *,
> > +                                         bool *changed);
> >  #endif /* controller/binding.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 386a64059..9ce9684e9 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -753,6 +753,7 @@ enum sb_engine_node {
> >      OVS_NODE(open_vswitch, "open_vswitch") \
> >      OVS_NODE(bridge, "bridge") \
> >      OVS_NODE(port, "port") \
> > +    OVS_NODE(interface, "interface") \
> >      OVS_NODE(qos, "qos")
> >
> >  enum ovs_engine_node {
> > @@ -974,6 +975,7 @@ struct ed_type_runtime_data {
> >      struct sset active_tunnels;
> >
> >      struct sset egress_ifaces;
> > +    struct smap local_iface_ids;
> >  };
> >
> >  static void *
> > @@ -987,6 +989,7 @@ en_runtime_data_init(struct engine_node *node
> OVS_UNUSED,
> >      sset_init(&data->local_lport_ids);
> >      sset_init(&data->active_tunnels);
> >      sset_init(&data->egress_ifaces);
> > +    smap_init(&data->local_iface_ids);
> >      local_bindings_init(&data->local_bindings);
> >      return data;
> >  }
> > @@ -1000,6 +1003,7 @@ en_runtime_data_cleanup(void *data)
> >      sset_destroy(&rt_data->local_lport_ids);
> >      sset_destroy(&rt_data->active_tunnels);
> >      sset_destroy(&rt_data->egress_ifaces);
> > +    smap_destroy(&rt_data->local_iface_ids);
> >      struct local_datapath *cur_node, *next_node;
> >      HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node,
> >                          &rt_data->local_datapaths) {
> > @@ -1041,6 +1045,10 @@ init_binding_ctx(struct engine_node *node,
> >          (struct ovsrec_port_table *)EN_OVSDB_GET(
> >              engine_get_input("OVS_port", node));
> >
> > +    struct ovsrec_interface_table *iface_table =
> > +        (struct ovsrec_interface_table *)EN_OVSDB_GET(
> > +            engine_get_input("OVS_interface", node));
> > +
> >      struct ovsrec_qos_table *qos_table =
> >          (struct ovsrec_qos_table *)EN_OVSDB_GET(
> >              engine_get_input("OVS_qos", node));
> > @@ -1070,6 +1078,7 @@ init_binding_ctx(struct engine_node *node,
> >      b_ctx_in->sbrec_port_binding_by_datapath =
> sbrec_port_binding_by_datapath;
> >      b_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> >      b_ctx_in->port_table = port_table;
> > +    b_ctx_in->iface_table = iface_table;
> >      b_ctx_in->qos_table = qos_table;
> >      b_ctx_in->port_binding_table = pb_table;
> >      b_ctx_in->br_int = br_int;
> > @@ -1083,6 +1092,7 @@ init_binding_ctx(struct engine_node *node,
> >      b_ctx_out->local_lport_ids = &rt_data->local_lport_ids;
> >      b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
> >      b_ctx_out->local_bindings = &rt_data->local_bindings;
> > +    b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
> >  }
> >
> >  static void
> > @@ -1111,10 +1121,12 @@ en_runtime_data_run(struct engine_node *node,
> void *data)
> >          sset_destroy(local_lport_ids);
> >          sset_destroy(active_tunnels);
> >          sset_destroy(&rt_data->egress_ifaces);
> > +        smap_destroy(&rt_data->local_iface_ids);
> >          sset_init(local_lports);
> >          sset_init(local_lport_ids);
> >          sset_init(active_tunnels);
> >          sset_init(&rt_data->egress_ifaces);
> > +        smap_init(&rt_data->local_iface_ids);
> >          local_bindings_init(&rt_data->local_bindings);
> >      }
> >
> > @@ -1140,6 +1152,34 @@ en_runtime_data_run(struct engine_node *node, void
> *data)
> >      engine_set_node_state(node, EN_UPDATED);
> >  }
> >
> > +static bool
> > +runtime_data_ovs_interface_handler(struct engine_node *node, void *data)
> > +{
> > +    struct ed_type_runtime_data *rt_data = data;
> > +    struct binding_ctx_in b_ctx_in;
> > +    struct binding_ctx_out b_ctx_out;
> > +    init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
> > +
> > +    bool changed = false;
> > +    if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out,
> > +                                              &changed)) {
> > +        return false;
> > +    }
> > +
> > +    if (changed) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static bool
> > +runtime_data_noop_handler(struct engine_node *node OVS_UNUSED,
> > +                          void *data OVS_UNUSED)
> > +{
> > +    return true;
> > +}
> > +
> >  static bool
> >  runtime_data_sb_port_binding_handler(struct engine_node *node, void
> *data)
> >  {
> > @@ -1147,11 +1187,21 @@ runtime_data_sb_port_binding_handler(struct
> engine_node *node, void *data)
> >      struct binding_ctx_in b_ctx_in;
> >      struct binding_ctx_out b_ctx_out;
> >      init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
> > +    if (!b_ctx_in.chassis_rec) {
> > +        return false;
> > +    }
> > +
> > +    bool changed = false;
> > +    if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out,
> > +                                             &changed)) {
> > +        return false;
> > +    }
> >
> > -    bool changed = binding_evaluate_port_binding_changes(&b_ctx_in,
> > -                                                         &b_ctx_out);
> > +    if (changed) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    }
> >
> > -    return !changed;
> > +    return true;
> >  }
> >
> >  /* Connection tracking zones. */
> > @@ -1895,7 +1945,10 @@ main(int argc, char *argv[])
> >
> >      engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, NULL);
> >      engine_add_input(&en_runtime_data, &en_ovs_bridge, NULL);
> > -    engine_add_input(&en_runtime_data, &en_ovs_port, NULL);
> > +    engine_add_input(&en_runtime_data, &en_ovs_port,
> > +                     runtime_data_noop_handler);
> > +    engine_add_input(&en_runtime_data, &en_ovs_interface,
> > +                     runtime_data_ovs_interface_handler);
> >      engine_add_input(&en_runtime_data, &en_ovs_qos, NULL);
> >
> >      engine_add_input(&en_runtime_data, &en_sb_chassis, NULL);
> > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> > index a8a15f8fe..5dfc6f7ca 100644
> > --- a/tests/ovn-performance.at
> > +++ b/tests/ovn-performance.at
> > @@ -239,6 +239,19 @@ for i in 1 2; do
> >      ovn_attach n1 br-phys 192.168.0.$i
> >  done
> >
> > +# Wait for the tunnel ports to be created and up.
> > +# Otherwise this may affect the lflow_run count.
> > +
> > +OVS_WAIT_UNTIL([
> > +    test $(as hv1 ovs-vsctl list interface ovn-hv2-0 | \
> > +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1
> > +])
> > +
> > +OVS_WAIT_UNTIL([
> > +    test $(as hv2 ovs-vsctl list interface ovn-hv1-0 | \
> > +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1
> > +])
> > +
> >  # Add router lr1
> >  OVN_CONTROLLER_EXPECT_HIT(
> >      [hv1 hv2], [lflow_run],
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list