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

Numan Siddique numans at ovn.org
Fri Mar 27 16:29:16 UTC 2020


On Wed, Mar 25, 2020 at 12:22 PM Han Zhou <hzhou at ovn.org> wrote:
>
> On Mon, Mar 16, 2020 at 4:16 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.
> >
> > Signed-off-by: Numan Siddique <numans at 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 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