[ovs-dev,ovn,3/6] ovn-controller: I-P for SB port binding and OVS interface in runtime_data.
diff mbox series

Message ID 20200316111543.1687608-1-numans@ovn.org
State New
Headers show
Series
  • Incremental processing improvements.
Related show

Commit Message

Numan Siddique March 16, 2020, 11:15 a.m. UTC
From: Numan Siddique <numans@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.

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/binding.c        | 467 ++++++++++++++++++++++++++++++------
 controller/binding.h        |   7 +-
 controller/ovn-controller.c |  55 ++++-
 3 files changed, 448 insertions(+), 81 deletions(-)

Comments

Han Zhou March 25, 2020, 6:51 a.m. UTC | #1
On Mon, Mar 16, 2020 at 4:16 AM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@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.
>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>  controller/binding.c        | 467 ++++++++++++++++++++++++++++++------
>  controller/binding.h        |   7 +-
>  controller/ovn-controller.c |  55 ++++-
>  3 files changed, 448 insertions(+), 81 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 34bd475de..ce4467f31 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -349,17 +349,6 @@ setup_qos(const char *egress_iface, struct hmap
*queue_map)
>      netdev_close(netdev_phy);
>  }
>
> -static void
> -update_local_lport_ids(struct sset *local_lport_ids,
> -                       const struct sbrec_port_binding *binding_rec)
> -{
> -        char buf[16];
> -        snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> -                 binding_rec->datapath->tunnel_key,
> -                 binding_rec->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
> @@ -490,6 +479,28 @@ 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 *binding_rec)
> +{
> +        char buf[16];
> +        snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> +                 binding_rec->datapath->tunnel_key,
> +                 binding_rec->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);
> +}
> +
>  enum local_binding_type {
>      BT_VIF,
>      BT_CHILD,
> @@ -545,18 +556,21 @@ local_binding_destroy(struct local_binding
*lbinding)
>      free(lbinding);
>  }
>
> +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);
> +}
> +
>  void
>  local_bindings_destroy(struct shash *local_bindings)
>  {
>      struct shash_node *node, *next;
>      SHASH_FOR_EACH_SAFE (node, next, local_bindings) {
>          struct local_binding *lbinding = node->data;
> -        struct local_binding *c, *n;
> -        LIST_FOR_EACH_SAFE (c, n, node, &lbinding->children) {
> -            ovs_list_remove(&c->node);
> -            free(c->name);
> -            free(c);
> -        }
> +        local_binding_destroy(lbinding);
>      }
>
>      shash_destroy(local_bindings);
> @@ -651,6 +665,22 @@ release_lport(const struct sbrec_port_binding *pb)
>      }
>  }
>
> +static void
> +release_local_binding_children(struct local_binding *lbinding)
> +{
> +    struct local_binding *l;
> +    LIST_FOR_EACH (l, node, &lbinding->children) {
> +        release_lport(l->pb);
> +    }
> +}
> +
> +static void
> +release_local_binding(struct local_binding *lbinding)
> +{
> +    release_local_binding_children(lbinding);
> +    release_lport(lbinding->pb);
> +}
> +
>  static void
>  consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
>                                struct binding_ctx_in *b_ctx_in,
> @@ -725,7 +755,6 @@ consider_port_binding(const struct sbrec_port_binding
*pb,
>                        struct binding_ctx_out *b_ctx_out,
>                        struct hmap *qos_map)
>  {
> -
>      bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, pb,
>                                        b_ctx_in->active_tunnels, NULL,
>                                        b_ctx_out->local_lports);
> @@ -821,6 +850,8 @@ build_local_bindings_from_local_ifaces(struct
binding_ctx_in *b_ctx_in,
>                      lbinding->pb = pb;
>                  }
>                  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. */
> @@ -938,62 +969,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
binding_ctx_out *b_ctx_out)
>      hmap_destroy(&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.
*/
> -bool
> -binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in,
> -                                      struct binding_ctx_out *b_ctx_out)
> -{
> -    if (!b_ctx_in->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;
> -            break;
> -        }
> -
> -        if (strcmp(binding_rec->type, "")) {
> -            changed = true;
> -            break;
> -        }
> -
> -        struct local_binding *lbinding = NULL;
> -        if (!binding_rec->type[0]) {
> -            if (!binding_rec->parent_port ||
!binding_rec->parent_port[0]) {
> -                lbinding = local_binding_find(b_ctx_out->local_bindings,
> -                                              binding_rec->logical_port);
> -            } else {
> -                lbinding = local_binding_find(b_ctx_out->local_bindings,
> -                                              binding_rec->parent_port);
> -            }
> -        }
> -
> -        if (lbinding) {
> -            changed = true;
> -            break;
> -        }
> -    }
> -
> -    return changed;
> -}
> -
>  /* Returns true if the database is all cleaned up, false if more work is
>   * required. */
>  bool
> @@ -1028,3 +1003,347 @@ binding_cleanup(struct ovsdb_idl_txn
*ovnsb_idl_txn,
>
>      return !any_changes;
>  }
> +
> +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)
> +{
> +    bool present = false;
> +    for (size_t i = 0; i < ld->n_peer_ports; i++) {
> +        if (ld->peer_ports[i].local == pb) {
> +            present = true;
> +            break;
> +        }
> +    }
> +
> +    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;
> +    }
> +
> +    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;
> +    }
> +
> +    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;
> +
> +    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_local_datapath_peer_port(peer, peer_ld, local_datapaths);
> +    }
> +}
> +
> +static void
> +update_local_datapath_for_pb(const struct sbrec_port_binding *pb,
> +                             struct binding_ctx_in *b_ctx_in,
> +                             struct binding_ctx_out *b_ctx_out,
> +                             struct local_datapath *ld)
> +{
> +    if (!strcmp(pb->type, "patch")) {
> +        add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld);
> +    } else if (!strcmp(pb->type, "localnet")) {
> +        struct shash bridge_mappings =
SHASH_INITIALIZER(&bridge_mappings);
> +        add_ovs_bridge_mappings(b_ctx_in->ovs_table,
b_ctx_in->bridge_table,
> +                                &bridge_mappings);
> +        consider_localnet_port(pb, &bridge_mappings,
b_ctx_out->egress_ifaces,
> +                               b_ctx_out->local_datapaths);
> +        shash_destroy(&bridge_mappings);
> +    } else if (!strcmp(pb->type, "l3gateway")) {
> +        const char *chassis_id = smap_get(&pb->options,
> +                                          "l3gateway-chassis");
> +        if (chassis_id && !strcmp(chassis_id,
b_ctx_in->chassis_rec->name)) {
> +            ld->has_local_l3gateway = true;
> +        }
> +    }
> +
> +    if (!strcmp(pb->type, "patch") ||
> +        !strcmp(pb->type, "localport") ||
> +        !strcmp(pb->type, "vtep")) {
> +        update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> +    }
> +}
> +
> +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;
> +        }
> +    }
> +}
> +
> +/* 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)
> +{
> +    if (!b_ctx_in->chassis_rec) {
> +        return false;
> +    }
> +
> +    bool handled = true;
> +    struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> +    struct hmap *qos_map_ptr =
> +        sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> +
> +    const struct ovsrec_interface *iface_rec;
> +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> +                                             b_ctx_in->iface_table) {
> +        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);
> +        if (iface_rec->type && iface_rec->type[0]) {
> +            handled = false;
> +            goto out;
> +        }
> +
> +        struct local_binding *lbinding = NULL;
> +        struct local_binding *claim_lbinding = NULL;
> +        const char *cleared_iface_id = NULL;
> +        int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
> +        if (!ovsrec_interface_is_deleted(iface_rec)) {
> +            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 (old_iface_id) {
> +                cleared_iface_id = old_iface_id;
> +            }
> +        } else {
> +            cleared_iface_id = iface_id;
> +            iface_id = NULL;
> +        }
> +
> +        if (cleared_iface_id) {
> +            lbinding = local_binding_find(b_ctx_out->local_bindings,
> +                                          cleared_iface_id);
> +            if (lbinding && lbinding->pb &&
> +                lbinding->pb->chassis == b_ctx_in->chassis_rec) {
> +                release_local_binding(lbinding);
> +                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);
> +                }
> +                local_binding_delete(b_ctx_out->local_bindings,
lbinding);
> +            }
> +
> +            sset_find_and_delete(b_ctx_out->local_lports,
cleared_iface_id);
> +            smap_remove(b_ctx_out->local_iface_ids, iface_rec->name);
> +        }
> +
> +        if (iface_id && ofport > 0) {
> +            sset_add(b_ctx_out->local_lports, iface_id);
> +            smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
> +                             iface_id);
> +            claim_lbinding =
> +                local_binding_find(b_ctx_out->local_bindings, iface_id);
> +            if (!claim_lbinding) {
> +                claim_lbinding = local_binding_create(iface_id,
iface_rec,
> +                                                      NULL, BT_VIF);
> +                local_binding_add(b_ctx_out->local_bindings,
claim_lbinding);
> +            } else {
> +                claim_lbinding->iface = iface_rec;
> +            }
> +
> +            if (!claim_lbinding->pb ||
> +                strcmp(claim_lbinding->name,
> +                       claim_lbinding->pb->logical_port)) {
> +                claim_lbinding->pb =
> +
 lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> +                                         claim_lbinding->name);
> +            }
> +
> +            if (claim_lbinding->pb) {
> +                consider_port_binding_for_vif(claim_lbinding->pb,
b_ctx_in,
> +                                              BT_VIF, claim_lbinding,
> +                                              b_ctx_out, qos_map_ptr);
> +            }
> +        }
> +    }
> +
> +    if (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);
> +        }
> +    }
> +
> +    hmap_destroy(&qos_map);
> +out:
> +    return handled;
> +}
> +
> +/* 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 updated = 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;
> +
> +    const struct sbrec_port_binding *pb;
> +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> +
b_ctx_in->port_binding_table) {
> +        bool consider_for_vif = false;
> +        struct local_binding *lbinding = NULL;
> +        enum local_binding_type binding_type = BT_VIF;
> +        bool is_deleted = sbrec_port_binding_is_deleted(pb);
> +        if (!pb->type[0]) {
> +            if (!pb->parent_port || !pb->parent_port[0]) {
> +                lbinding = local_binding_find(b_ctx_out->local_bindings,
> +                                              pb->logical_port);
> +            } else {
> +                lbinding = local_binding_find(b_ctx_out->local_bindings,
> +                                              pb->parent_port);
> +                binding_type = BT_CHILD;
> +            }
> +
> +            consider_for_vif = true;
> +        } else if (!strcmp(pb->type, "virtual") &&
> +                   pb->virtual_parent && pb->virtual_parent[0]) {
> +            lbinding = local_binding_find(b_ctx_out->local_bindings,
> +                                          pb->virtual_parent);
> +            consider_for_vif = true;
> +            binding_type = BT_VIRTUAL;
> +        }
> +
> +        if (is_deleted) {
> +            if (lbinding) {
> +                updated = true;
> +                lbinding->pb = NULL;
> +
> +                if (binding_type == BT_VIF) {
> +                    release_local_binding_children(lbinding);
> +                }
> +            }
> +
> +            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);
> +            }
> +        } else {
> +            if (consider_for_vif) {
> +                if (lbinding) {
> +                    updated = true;
> +                    lbinding->pb = pb;
> +                    consider_port_binding_for_vif(pb, b_ctx_in,
binding_type,
> +                                                  lbinding, b_ctx_out,
> +                                                  qos_map_ptr);
> +                }
> +            } else {
> +                updated = true;
> +                consider_port_binding(pb, b_ctx_in, b_ctx_out,
qos_map_ptr);
> +                struct local_datapath *ld =
> +                    get_local_datapath(b_ctx_out->local_datapaths,
> +                                       pb->datapath->tunnel_key);
> +                if (ld) {
> +                    update_local_datapath_for_pb(pb, b_ctx_in,
b_ctx_out, ld);
> +                }
> +            }
> +        }
> +    }
> +
> +    return updated;
> +}
> diff --git a/controller/binding.h b/controller/binding.h
> index 6bee1d12e..95ca0367d 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -56,6 +56,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 *);
> @@ -63,10 +64,12 @@ 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_destroy(struct shash *local_bindings);
>  void binding_add_vport_to_local_bindings(
>      struct shash *local_bindings, const struct sbrec_port_binding
*parent,
>      const struct sbrec_port_binding *vport);
> +bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
> +                                          struct binding_ctx_out *);
> +bool binding_handle_port_binding_changes(struct binding_ctx_in *,
> +                                         struct binding_ctx_out *);
>  #endif /* controller/binding.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 8cc27cebf..6841be29d 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 *
> @@ -988,6 +990,7 @@ en_runtime_data_init(struct engine_node *node
OVS_UNUSED,
>      sset_init(&data->active_tunnels);
>      sset_init(&data->egress_ifaces);
>      shash_init(&data->local_bindings);
> +    smap_init(&data->local_iface_ids);
>      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_init(&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
> @@ -1110,10 +1120,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);
>      }
>
>      struct binding_ctx_in b_ctx_in;
> @@ -1139,17 +1151,47 @@ en_runtime_data_run(struct engine_node *node,
void *data)
>  }
>
>  static bool
> -runtime_data_sb_port_binding_handler(struct engine_node *node, void
*data)
> +runtime_data_ovs_interface_handler(struct engine_node *node, void *data)
>  {
> +    if (!engine_get_context()->ovnsb_idl_txn) {
> +        return false;
> +    }
> +
>      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 = binding_evaluate_port_binding_changes(&b_ctx_in,
> -                                                         &b_ctx_out);
> +    engine_set_node_state(node, EN_UPDATED);
> +    return binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out);
> +}
> +
> +static bool
> +runtime_data_noop_handler(struct engine_node *node OVS_UNUSED,
> +                          void *data OVS_UNUSED)
> +{
> +    return true;
> +}
>
> -    return !changed;
> +static bool
> +runtime_data_sb_port_binding_handler(struct engine_node *node, void
*data)
> +{
> +    if (!engine_get_context()->ovnsb_idl_txn) {
> +        return false;
> +    }
> +
> +    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);
> +    if (!b_ctx_in.chassis_rec) {
> +        return false;
> +    }
> +
> +    bool updated = binding_handle_port_binding_changes(&b_ctx_in,
&b_ctx_out);
> +    enum engine_node_state state = updated ? EN_UPDATED : EN_VALID;

An input change handler should never set the node's state to EN_VALID,
because it doesn't know if any other input handler already set the state to
EN_UPDATED. What it should do is just set state to EN_UPDATE if it knows
that the data is changed. (Sorry this was not well documented, and there's
already existing code like this in address-set handling. This was a miss in
a previous review of a refactor of the I-P code.)

> +    engine_set_node_state(node, state);
> +    return true;
>  }
>
>  /* Connection tracking zones. */
> @@ -1891,7 +1933,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);
> --
> 2.24.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Numan Siddique March 27, 2020, 4:29 p.m. UTC | #2
On Wed, Mar 25, 2020 at 12:22 PM Han Zhou <hzhou@ovn.org> wrote:
>
> On Mon, Mar 16, 2020 at 4:16 AM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@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.
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >  controller/binding.c        | 467 ++++++++++++++++++++++++++++++------
> >  controller/binding.h        |   7 +-
> >  controller/ovn-controller.c |  55 ++++-
> >  3 files changed, 448 insertions(+), 81 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 34bd475de..ce4467f31 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -349,17 +349,6 @@ setup_qos(const char *egress_iface, struct hmap
> *queue_map)
> >      netdev_close(netdev_phy);
> >  }
> >
> > -static void
> > -update_local_lport_ids(struct sset *local_lport_ids,
> > -                       const struct sbrec_port_binding *binding_rec)
> > -{
> > -        char buf[16];
> > -        snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> > -                 binding_rec->datapath->tunnel_key,
> > -                 binding_rec->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
> > @@ -490,6 +479,28 @@ 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 *binding_rec)
> > +{
> > +        char buf[16];
> > +        snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> > +                 binding_rec->datapath->tunnel_key,
> > +                 binding_rec->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);
> > +}
> > +
> >  enum local_binding_type {
> >      BT_VIF,
> >      BT_CHILD,
> > @@ -545,18 +556,21 @@ local_binding_destroy(struct local_binding
> *lbinding)
> >      free(lbinding);
> >  }
> >
> > +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);
> > +}
> > +
> >  void
> >  local_bindings_destroy(struct shash *local_bindings)
> >  {
> >      struct shash_node *node, *next;
> >      SHASH_FOR_EACH_SAFE (node, next, local_bindings) {
> >          struct local_binding *lbinding = node->data;
> > -        struct local_binding *c, *n;
> > -        LIST_FOR_EACH_SAFE (c, n, node, &lbinding->children) {
> > -            ovs_list_remove(&c->node);
> > -            free(c->name);
> > -            free(c);
> > -        }
> > +        local_binding_destroy(lbinding);
> >      }
> >
> >      shash_destroy(local_bindings);
> > @@ -651,6 +665,22 @@ release_lport(const struct sbrec_port_binding *pb)
> >      }
> >  }
> >
> > +static void
> > +release_local_binding_children(struct local_binding *lbinding)
> > +{
> > +    struct local_binding *l;
> > +    LIST_FOR_EACH (l, node, &lbinding->children) {
> > +        release_lport(l->pb);
> > +    }
> > +}
> > +
> > +static void
> > +release_local_binding(struct local_binding *lbinding)
> > +{
> > +    release_local_binding_children(lbinding);
> > +    release_lport(lbinding->pb);
> > +}
> > +
> >  static void
> >  consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
> >                                struct binding_ctx_in *b_ctx_in,
> > @@ -725,7 +755,6 @@ consider_port_binding(const struct sbrec_port_binding
> *pb,
> >                        struct binding_ctx_out *b_ctx_out,
> >                        struct hmap *qos_map)
> >  {
> > -
> >      bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, pb,
> >                                        b_ctx_in->active_tunnels, NULL,
> >                                        b_ctx_out->local_lports);
> > @@ -821,6 +850,8 @@ build_local_bindings_from_local_ifaces(struct
> binding_ctx_in *b_ctx_in,
> >                      lbinding->pb = pb;
> >                  }
> >                  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. */
> > @@ -938,62 +969,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
> binding_ctx_out *b_ctx_out)
> >      hmap_destroy(&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.
> */
> > -bool
> > -binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in,
> > -                                      struct binding_ctx_out *b_ctx_out)
> > -{
> > -    if (!b_ctx_in->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;
> > -            break;
> > -        }
> > -
> > -        if (strcmp(binding_rec->type, "")) {
> > -            changed = true;
> > -            break;
> > -        }
> > -
> > -        struct local_binding *lbinding = NULL;
> > -        if (!binding_rec->type[0]) {
> > -            if (!binding_rec->parent_port ||
> !binding_rec->parent_port[0]) {
> > -                lbinding = local_binding_find(b_ctx_out->local_bindings,
> > -                                              binding_rec->logical_port);
> > -            } else {
> > -                lbinding = local_binding_find(b_ctx_out->local_bindings,
> > -                                              binding_rec->parent_port);
> > -            }
> > -        }
> > -
> > -        if (lbinding) {
> > -            changed = true;
> > -            break;
> > -        }
> > -    }
> > -
> > -    return changed;
> > -}
> > -
> >  /* Returns true if the database is all cleaned up, false if more work is
> >   * required. */
> >  bool
> > @@ -1028,3 +1003,347 @@ binding_cleanup(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> >
> >      return !any_changes;
> >  }
> > +
> > +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)
> > +{
> > +    bool present = false;
> > +    for (size_t i = 0; i < ld->n_peer_ports; i++) {
> > +        if (ld->peer_ports[i].local == pb) {
> > +            present = true;
> > +            break;
> > +        }
> > +    }
> > +
> > +    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;
> > +    }
> > +
> > +    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;
> > +    }
> > +
> > +    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;
> > +
> > +    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_local_datapath_peer_port(peer, peer_ld, local_datapaths);
> > +    }
> > +}
> > +
> > +static void
> > +update_local_datapath_for_pb(const struct sbrec_port_binding *pb,
> > +                             struct binding_ctx_in *b_ctx_in,
> > +                             struct binding_ctx_out *b_ctx_out,
> > +                             struct local_datapath *ld)
> > +{
> > +    if (!strcmp(pb->type, "patch")) {
> > +        add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld);
> > +    } else if (!strcmp(pb->type, "localnet")) {
> > +        struct shash bridge_mappings =
> SHASH_INITIALIZER(&bridge_mappings);
> > +        add_ovs_bridge_mappings(b_ctx_in->ovs_table,
> b_ctx_in->bridge_table,
> > +                                &bridge_mappings);
> > +        consider_localnet_port(pb, &bridge_mappings,
> b_ctx_out->egress_ifaces,
> > +                               b_ctx_out->local_datapaths);
> > +        shash_destroy(&bridge_mappings);
> > +    } else if (!strcmp(pb->type, "l3gateway")) {
> > +        const char *chassis_id = smap_get(&pb->options,
> > +                                          "l3gateway-chassis");
> > +        if (chassis_id && !strcmp(chassis_id,
> b_ctx_in->chassis_rec->name)) {
> > +            ld->has_local_l3gateway = true;
> > +        }
> > +    }
> > +
> > +    if (!strcmp(pb->type, "patch") ||
> > +        !strcmp(pb->type, "localport") ||
> > +        !strcmp(pb->type, "vtep")) {
> > +        update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
> > +    }
> > +}
> > +
> > +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;
> > +        }
> > +    }
> > +}
> > +
> > +/* 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)
> > +{
> > +    if (!b_ctx_in->chassis_rec) {
> > +        return false;
> > +    }
> > +
> > +    bool handled = true;
> > +    struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > +    struct hmap *qos_map_ptr =
> > +        sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > +
> > +    const struct ovsrec_interface *iface_rec;
> > +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
> > +                                             b_ctx_in->iface_table) {
> > +        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);
> > +        if (iface_rec->type && iface_rec->type[0]) {
> > +            handled = false;
> > +            goto out;
> > +        }
> > +
> > +        struct local_binding *lbinding = NULL;
> > +        struct local_binding *claim_lbinding = NULL;
> > +        const char *cleared_iface_id = NULL;
> > +        int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
> > +        if (!ovsrec_interface_is_deleted(iface_rec)) {
> > +            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 (old_iface_id) {
> > +                cleared_iface_id = old_iface_id;
> > +            }
> > +        } else {
> > +            cleared_iface_id = iface_id;
> > +            iface_id = NULL;
> > +        }
> > +
> > +        if (cleared_iface_id) {
> > +            lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                          cleared_iface_id);
> > +            if (lbinding && lbinding->pb &&
> > +                lbinding->pb->chassis == b_ctx_in->chassis_rec) {
> > +                release_local_binding(lbinding);
> > +                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);
> > +                }
> > +                local_binding_delete(b_ctx_out->local_bindings,
> lbinding);
> > +            }
> > +
> > +            sset_find_and_delete(b_ctx_out->local_lports,
> cleared_iface_id);
> > +            smap_remove(b_ctx_out->local_iface_ids, iface_rec->name);
> > +        }
> > +
> > +        if (iface_id && ofport > 0) {
> > +            sset_add(b_ctx_out->local_lports, iface_id);
> > +            smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
> > +                             iface_id);
> > +            claim_lbinding =
> > +                local_binding_find(b_ctx_out->local_bindings, iface_id);
> > +            if (!claim_lbinding) {
> > +                claim_lbinding = local_binding_create(iface_id,
> iface_rec,
> > +                                                      NULL, BT_VIF);
> > +                local_binding_add(b_ctx_out->local_bindings,
> claim_lbinding);
> > +            } else {
> > +                claim_lbinding->iface = iface_rec;
> > +            }
> > +
> > +            if (!claim_lbinding->pb ||
> > +                strcmp(claim_lbinding->name,
> > +                       claim_lbinding->pb->logical_port)) {
> > +                claim_lbinding->pb =
> > +
>  lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> > +                                         claim_lbinding->name);
> > +            }
> > +
> > +            if (claim_lbinding->pb) {
> > +                consider_port_binding_for_vif(claim_lbinding->pb,
> b_ctx_in,
> > +                                              BT_VIF, claim_lbinding,
> > +                                              b_ctx_out, qos_map_ptr);
> > +            }
> > +        }
> > +    }
> > +
> > +    if (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);
> > +        }
> > +    }
> > +
> > +    hmap_destroy(&qos_map);
> > +out:
> > +    return handled;
> > +}
> > +
> > +/* 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 updated = 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;
> > +
> > +    const struct sbrec_port_binding *pb;
> > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > +
> b_ctx_in->port_binding_table) {
> > +        bool consider_for_vif = false;
> > +        struct local_binding *lbinding = NULL;
> > +        enum local_binding_type binding_type = BT_VIF;
> > +        bool is_deleted = sbrec_port_binding_is_deleted(pb);
> > +        if (!pb->type[0]) {
> > +            if (!pb->parent_port || !pb->parent_port[0]) {
> > +                lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                              pb->logical_port);
> > +            } else {
> > +                lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                              pb->parent_port);
> > +                binding_type = BT_CHILD;
> > +            }
> > +
> > +            consider_for_vif = true;
> > +        } else if (!strcmp(pb->type, "virtual") &&
> > +                   pb->virtual_parent && pb->virtual_parent[0]) {
> > +            lbinding = local_binding_find(b_ctx_out->local_bindings,
> > +                                          pb->virtual_parent);
> > +            consider_for_vif = true;
> > +            binding_type = BT_VIRTUAL;
> > +        }
> > +
> > +        if (is_deleted) {
> > +            if (lbinding) {
> > +                updated = true;
> > +                lbinding->pb = NULL;
> > +
> > +                if (binding_type == BT_VIF) {
> > +                    release_local_binding_children(lbinding);
> > +                }
> > +            }
> > +
> > +            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);
> > +            }
> > +        } else {
> > +            if (consider_for_vif) {
> > +                if (lbinding) {
> > +                    updated = true;
> > +                    lbinding->pb = pb;
> > +                    consider_port_binding_for_vif(pb, b_ctx_in,
> binding_type,
> > +                                                  lbinding, b_ctx_out,
> > +                                                  qos_map_ptr);
> > +                }
> > +            } else {
> > +                updated = true;
> > +                consider_port_binding(pb, b_ctx_in, b_ctx_out,
> qos_map_ptr);
> > +                struct local_datapath *ld =
> > +                    get_local_datapath(b_ctx_out->local_datapaths,
> > +                                       pb->datapath->tunnel_key);
> > +                if (ld) {
> > +                    update_local_datapath_for_pb(pb, b_ctx_in,
> b_ctx_out, ld);
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    return updated;
> > +}
> > diff --git a/controller/binding.h b/controller/binding.h
> > index 6bee1d12e..95ca0367d 100644
> > --- a/controller/binding.h
> > +++ b/controller/binding.h
> > @@ -56,6 +56,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 *);
> > @@ -63,10 +64,12 @@ 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_destroy(struct shash *local_bindings);
> >  void binding_add_vport_to_local_bindings(
> >      struct shash *local_bindings, const struct sbrec_port_binding
> *parent,
> >      const struct sbrec_port_binding *vport);
> > +bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
> > +                                          struct binding_ctx_out *);
> > +bool binding_handle_port_binding_changes(struct binding_ctx_in *,
> > +                                         struct binding_ctx_out *);
> >  #endif /* controller/binding.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 8cc27cebf..6841be29d 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 *
> > @@ -988,6 +990,7 @@ en_runtime_data_init(struct engine_node *node
> OVS_UNUSED,
> >      sset_init(&data->active_tunnels);
> >      sset_init(&data->egress_ifaces);
> >      shash_init(&data->local_bindings);
> > +    smap_init(&data->local_iface_ids);
> >      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_init(&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
> > @@ -1110,10 +1120,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);
> >      }
> >
> >      struct binding_ctx_in b_ctx_in;
> > @@ -1139,17 +1151,47 @@ en_runtime_data_run(struct engine_node *node,
> void *data)
> >  }
> >
> >  static bool
> > -runtime_data_sb_port_binding_handler(struct engine_node *node, void
> *data)
> > +runtime_data_ovs_interface_handler(struct engine_node *node, void *data)
> >  {
> > +    if (!engine_get_context()->ovnsb_idl_txn) {
> > +        return false;
> > +    }
> > +
> >      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 = binding_evaluate_port_binding_changes(&b_ctx_in,
> > -                                                         &b_ctx_out);
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out);
> > +}
> > +
> > +static bool
> > +runtime_data_noop_handler(struct engine_node *node OVS_UNUSED,
> > +                          void *data OVS_UNUSED)
> > +{
> > +    return true;
> > +}
> >
> > -    return !changed;
> > +static bool
> > +runtime_data_sb_port_binding_handler(struct engine_node *node, void
> *data)
> > +{
> > +    if (!engine_get_context()->ovnsb_idl_txn) {
> > +        return false;
> > +    }
> > +
> > +    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);
> > +    if (!b_ctx_in.chassis_rec) {
> > +        return false;
> > +    }
> > +
> > +    bool updated = binding_handle_port_binding_changes(&b_ctx_in,
> &b_ctx_out);
> > +    enum engine_node_state state = updated ? EN_UPDATED : EN_VALID;
>
> An input change handler should never set the node's state to EN_VALID,
> because it doesn't know if any other input handler already set the state to
> EN_UPDATED. What it should do is just set state to EN_UPDATE if it knows
> that the data is changed. (Sorry this was not well documented, and there's
> already existing code like this in address-set handling. This was a miss in
> a previous review of a refactor of the I-P code.)

Makes sense to me. I'll update it in v2.

Thanks
Numan

Numan

>
> > +    engine_set_node_state(node, state);
> > +    return true;
> >  }
> >
> >  /* Connection tracking zones. */
> > @@ -1891,7 +1933,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);
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch
diff mbox series

diff --git a/controller/binding.c b/controller/binding.c
index 34bd475de..ce4467f31 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -349,17 +349,6 @@  setup_qos(const char *egress_iface, struct hmap *queue_map)
     netdev_close(netdev_phy);
 }
 
-static void
-update_local_lport_ids(struct sset *local_lport_ids,
-                       const struct sbrec_port_binding *binding_rec)
-{
-        char buf[16];
-        snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
-                 binding_rec->datapath->tunnel_key,
-                 binding_rec->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
@@ -490,6 +479,28 @@  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 *binding_rec)
+{
+        char buf[16];
+        snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
+                 binding_rec->datapath->tunnel_key,
+                 binding_rec->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);
+}
+
 enum local_binding_type {
     BT_VIF,
     BT_CHILD,
@@ -545,18 +556,21 @@  local_binding_destroy(struct local_binding *lbinding)
     free(lbinding);
 }
 
+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);
+}
+
 void
 local_bindings_destroy(struct shash *local_bindings)
 {
     struct shash_node *node, *next;
     SHASH_FOR_EACH_SAFE (node, next, local_bindings) {
         struct local_binding *lbinding = node->data;
-        struct local_binding *c, *n;
-        LIST_FOR_EACH_SAFE (c, n, node, &lbinding->children) {
-            ovs_list_remove(&c->node);
-            free(c->name);
-            free(c);
-        }
+        local_binding_destroy(lbinding);
     }
 
     shash_destroy(local_bindings);
@@ -651,6 +665,22 @@  release_lport(const struct sbrec_port_binding *pb)
     }
 }
 
+static void
+release_local_binding_children(struct local_binding *lbinding)
+{
+    struct local_binding *l;
+    LIST_FOR_EACH (l, node, &lbinding->children) {
+        release_lport(l->pb);
+    }
+}
+
+static void
+release_local_binding(struct local_binding *lbinding)
+{
+    release_local_binding_children(lbinding);
+    release_lport(lbinding->pb);
+}
+
 static void
 consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
                               struct binding_ctx_in *b_ctx_in,
@@ -725,7 +755,6 @@  consider_port_binding(const struct sbrec_port_binding *pb,
                       struct binding_ctx_out *b_ctx_out,
                       struct hmap *qos_map)
 {
-
     bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, pb,
                                       b_ctx_in->active_tunnels, NULL,
                                       b_ctx_out->local_lports);
@@ -821,6 +850,8 @@  build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in,
                     lbinding->pb = pb;
                 }
                 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. */
@@ -938,62 +969,6 @@  binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
     hmap_destroy(&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. */
-bool
-binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in,
-                                      struct binding_ctx_out *b_ctx_out)
-{
-    if (!b_ctx_in->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;
-            break;
-        }
-
-        if (strcmp(binding_rec->type, "")) {
-            changed = true;
-            break;
-        }
-
-        struct local_binding *lbinding = NULL;
-        if (!binding_rec->type[0]) {
-            if (!binding_rec->parent_port || !binding_rec->parent_port[0]) {
-                lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                              binding_rec->logical_port);
-            } else {
-                lbinding = local_binding_find(b_ctx_out->local_bindings,
-                                              binding_rec->parent_port);
-            }
-        }
-
-        if (lbinding) {
-            changed = true;
-            break;
-        }
-    }
-
-    return changed;
-}
-
 /* Returns true if the database is all cleaned up, false if more work is
  * required. */
 bool
@@ -1028,3 +1003,347 @@  binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
 
     return !any_changes;
 }
+
+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)
+{
+    bool present = false;
+    for (size_t i = 0; i < ld->n_peer_ports; i++) {
+        if (ld->peer_ports[i].local == pb) {
+            present = true;
+            break;
+        }
+    }
+
+    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;
+    }
+
+    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;
+    }
+
+    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;
+
+    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_local_datapath_peer_port(peer, peer_ld, local_datapaths);
+    }
+}
+
+static void
+update_local_datapath_for_pb(const struct sbrec_port_binding *pb,
+                             struct binding_ctx_in *b_ctx_in,
+                             struct binding_ctx_out *b_ctx_out,
+                             struct local_datapath *ld)
+{
+    if (!strcmp(pb->type, "patch")) {
+        add_local_datapath_peer_port(pb, b_ctx_in, b_ctx_out, ld);
+    } else if (!strcmp(pb->type, "localnet")) {
+        struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
+        add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table,
+                                &bridge_mappings);
+        consider_localnet_port(pb, &bridge_mappings, b_ctx_out->egress_ifaces,
+                               b_ctx_out->local_datapaths);
+        shash_destroy(&bridge_mappings);
+    } else if (!strcmp(pb->type, "l3gateway")) {
+        const char *chassis_id = smap_get(&pb->options,
+                                          "l3gateway-chassis");
+        if (chassis_id && !strcmp(chassis_id, b_ctx_in->chassis_rec->name)) {
+            ld->has_local_l3gateway = true;
+        }
+    }
+
+    if (!strcmp(pb->type, "patch") ||
+        !strcmp(pb->type, "localport") ||
+        !strcmp(pb->type, "vtep")) {
+        update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
+    }
+}
+
+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;
+        }
+    }
+}
+
+/* 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)
+{
+    if (!b_ctx_in->chassis_rec) {
+        return false;
+    }
+
+    bool handled = true;
+    struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
+    struct hmap *qos_map_ptr =
+        sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
+
+    const struct ovsrec_interface *iface_rec;
+    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
+                                             b_ctx_in->iface_table) {
+        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);
+        if (iface_rec->type && iface_rec->type[0]) {
+            handled = false;
+            goto out;
+        }
+
+        struct local_binding *lbinding = NULL;
+        struct local_binding *claim_lbinding = NULL;
+        const char *cleared_iface_id = NULL;
+        int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
+        if (!ovsrec_interface_is_deleted(iface_rec)) {
+            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 (old_iface_id) {
+                cleared_iface_id = old_iface_id;
+            }
+        } else {
+            cleared_iface_id = iface_id;
+            iface_id = NULL;
+        }
+
+        if (cleared_iface_id) {
+            lbinding = local_binding_find(b_ctx_out->local_bindings,
+                                          cleared_iface_id);
+            if (lbinding && lbinding->pb &&
+                lbinding->pb->chassis == b_ctx_in->chassis_rec) {
+                release_local_binding(lbinding);
+                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);
+                }
+                local_binding_delete(b_ctx_out->local_bindings, lbinding);
+            }
+
+            sset_find_and_delete(b_ctx_out->local_lports, cleared_iface_id);
+            smap_remove(b_ctx_out->local_iface_ids, iface_rec->name);
+        }
+
+        if (iface_id && ofport > 0) {
+            sset_add(b_ctx_out->local_lports, iface_id);
+            smap_replace(b_ctx_out->local_iface_ids, iface_rec->name,
+                             iface_id);
+            claim_lbinding =
+                local_binding_find(b_ctx_out->local_bindings, iface_id);
+            if (!claim_lbinding) {
+                claim_lbinding = local_binding_create(iface_id, iface_rec,
+                                                      NULL, BT_VIF);
+                local_binding_add(b_ctx_out->local_bindings, claim_lbinding);
+            } else {
+                claim_lbinding->iface = iface_rec;
+            }
+
+            if (!claim_lbinding->pb ||
+                strcmp(claim_lbinding->name,
+                       claim_lbinding->pb->logical_port)) {
+                claim_lbinding->pb =
+                    lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
+                                         claim_lbinding->name);
+            }
+
+            if (claim_lbinding->pb) {
+                consider_port_binding_for_vif(claim_lbinding->pb, b_ctx_in,
+                                              BT_VIF, claim_lbinding,
+                                              b_ctx_out, qos_map_ptr);
+            }
+        }
+    }
+
+    if (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);
+        }
+    }
+
+    hmap_destroy(&qos_map);
+out:
+    return handled;
+}
+
+/* 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 updated = 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;
+
+    const struct sbrec_port_binding *pb;
+    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
+                                               b_ctx_in->port_binding_table) {
+        bool consider_for_vif = false;
+        struct local_binding *lbinding = NULL;
+        enum local_binding_type binding_type = BT_VIF;
+        bool is_deleted = sbrec_port_binding_is_deleted(pb);
+        if (!pb->type[0]) {
+            if (!pb->parent_port || !pb->parent_port[0]) {
+                lbinding = local_binding_find(b_ctx_out->local_bindings,
+                                              pb->logical_port);
+            } else {
+                lbinding = local_binding_find(b_ctx_out->local_bindings,
+                                              pb->parent_port);
+                binding_type = BT_CHILD;
+            }
+
+            consider_for_vif = true;
+        } else if (!strcmp(pb->type, "virtual") &&
+                   pb->virtual_parent && pb->virtual_parent[0]) {
+            lbinding = local_binding_find(b_ctx_out->local_bindings,
+                                          pb->virtual_parent);
+            consider_for_vif = true;
+            binding_type = BT_VIRTUAL;
+        }
+
+        if (is_deleted) {
+            if (lbinding) {
+                updated = true;
+                lbinding->pb = NULL;
+
+                if (binding_type == BT_VIF) {
+                    release_local_binding_children(lbinding);
+                }
+            }
+
+            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);
+            }
+        } else {
+            if (consider_for_vif) {
+                if (lbinding) {
+                    updated = true;
+                    lbinding->pb = pb;
+                    consider_port_binding_for_vif(pb, b_ctx_in, binding_type,
+                                                  lbinding, b_ctx_out,
+                                                  qos_map_ptr);
+                }
+            } else {
+                updated = true;
+                consider_port_binding(pb, b_ctx_in, b_ctx_out, qos_map_ptr);
+                struct local_datapath *ld =
+                    get_local_datapath(b_ctx_out->local_datapaths,
+                                       pb->datapath->tunnel_key);
+                if (ld) {
+                    update_local_datapath_for_pb(pb, b_ctx_in, b_ctx_out, ld);
+                }
+            }
+        }
+    }
+
+    return updated;
+}
diff --git a/controller/binding.h b/controller/binding.h
index 6bee1d12e..95ca0367d 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -56,6 +56,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 *);
@@ -63,10 +64,12 @@  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_destroy(struct shash *local_bindings);
 void binding_add_vport_to_local_bindings(
     struct shash *local_bindings, const struct sbrec_port_binding *parent,
     const struct sbrec_port_binding *vport);
+bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
+                                          struct binding_ctx_out *);
+bool binding_handle_port_binding_changes(struct binding_ctx_in *,
+                                         struct binding_ctx_out *);
 #endif /* controller/binding.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 8cc27cebf..6841be29d 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 *
@@ -988,6 +990,7 @@  en_runtime_data_init(struct engine_node *node OVS_UNUSED,
     sset_init(&data->active_tunnels);
     sset_init(&data->egress_ifaces);
     shash_init(&data->local_bindings);
+    smap_init(&data->local_iface_ids);
     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_init(&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
@@ -1110,10 +1120,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);
     }
 
     struct binding_ctx_in b_ctx_in;
@@ -1139,17 +1151,47 @@  en_runtime_data_run(struct engine_node *node, void *data)
 }
 
 static bool
-runtime_data_sb_port_binding_handler(struct engine_node *node, void *data)
+runtime_data_ovs_interface_handler(struct engine_node *node, void *data)
 {
+    if (!engine_get_context()->ovnsb_idl_txn) {
+        return false;
+    }
+
     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 = binding_evaluate_port_binding_changes(&b_ctx_in,
-                                                         &b_ctx_out);
+    engine_set_node_state(node, EN_UPDATED);
+    return binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out);
+}
+
+static bool
+runtime_data_noop_handler(struct engine_node *node OVS_UNUSED,
+                          void *data OVS_UNUSED)
+{
+    return true;
+}
 
-    return !changed;
+static bool
+runtime_data_sb_port_binding_handler(struct engine_node *node, void *data)
+{
+    if (!engine_get_context()->ovnsb_idl_txn) {
+        return false;
+    }
+
+    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);
+    if (!b_ctx_in.chassis_rec) {
+        return false;
+    }
+
+    bool updated = binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out);
+    enum engine_node_state state = updated ? EN_UPDATED : EN_VALID;
+    engine_set_node_state(node, state);
+    return true;
 }
 
 /* Connection tracking zones. */
@@ -1891,7 +1933,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);