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

Dumitru Ceara dceara at redhat.com
Tue May 12 15:27:48 UTC 2020


On 5/11/20 5:46 PM, Dumitru Ceara wrote:
> On 5/11/20 11:45 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.
> 
> Hi Numan,
> 
> This is not a full review as I only went through half of the changes in
> this patch but please find some initial comments inline.
> 
> Thanks,
> Dumitru
> 
>>
>> Signed-off-by: Numan Siddique <numans at ovn.org>
>> ---
>>  controller/binding.c        | 602 ++++++++++++++++++++++++++++++------
>>  controller/binding.h        |   9 +-
>>  controller/ovn-controller.c |  61 +++-
>>  tests/ovn-performance.at    |  13 +
>>  4 files changed, 593 insertions(+), 92 deletions(-)
>>
>> diff --git a/controller/binding.c b/controller/binding.c
>> index e35440c78..662424518 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
>> @@ -488,6 +477,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,
>> @@ -530,18 +541,34 @@ local_binding_find(struct shash *local_bindings, const char *name)
>>      return shash_find_data(local_bindings, name);
>>  }
>>  
>> +static void
>> +local_binding_destroy(struct local_binding *lbinding)
>> +{
>> +    struct local_binding *c, *next;
>> +    LIST_FOR_EACH_SAFE (c, next, node, &lbinding->children) {
>> +        ovs_list_remove(&c->node);
>> +        free(c->name);
>> +        free(c);
>> +    }
>> +    free(lbinding->name);
>> +    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);
>> @@ -594,10 +621,11 @@ binding_add_vport_to_local_bindings(struct shash *local_bindings,
>>      }
>>  }
>>  
>> -static void
>> +static bool
>>  claim_lport(const struct sbrec_port_binding *pb,
>>              const struct sbrec_chassis *chassis_rec,
>> -            const struct ovsrec_interface *iface_rec)
>> +            const struct ovsrec_interface *iface_rec,
>> +            bool cant_update_sb)
> 
> I think "sb_readonly" would be more explicit than "cant_update_sb".
> 
>>  {
>>      if (pb->chassis != chassis_rec) {
>>          if (pb->chassis) {
>> @@ -611,6 +639,9 @@ claim_lport(const struct sbrec_port_binding *pb,
>>              VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
>>          }
>>  
>> +        if (cant_update_sb) {
>> +            return false;
>> +        }
>>          sbrec_port_binding_set_chassis(pb, chassis_rec);
>>      }
>>  
>> @@ -618,25 +649,74 @@ claim_lport(const struct sbrec_port_binding *pb,
>>      struct sbrec_encap *encap_rec =
>>          sbrec_get_port_encap(chassis_rec, iface_rec);
>>      if (encap_rec && pb->encap != encap_rec) {
>> +        if (cant_update_sb) {
>> +            return false;
>> +        }
>>          sbrec_port_binding_set_encap(pb, encap_rec);
>>      }
>> +
>> +    return true;
>>  }
>>  
>> -static void
>> -release_lport(const struct sbrec_port_binding *pb)
>> +static bool
>> +release_lport(const struct sbrec_port_binding *pb, bool cant_update_sb)
> 
> Same here.
> 
>>  {
>>      VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
>>      if (pb->encap) {
>> -        sbrec_port_binding_set_encap(pb, NULL);
>> +        if (pb->encap) {
> 
> I guess this is a typo, i.e., "if (pb->encap) { if (pb->encap) {".
> 
>> +            if (cant_update_sb) {
>> +                return false;
>> +            }
>> +            sbrec_port_binding_set_encap(pb, NULL);
>> +        }
>> +    }
>> +
>> +    if (pb->chassis) {
>> +        if (cant_update_sb) {
>> +            return false;
>> +        }
>> +        sbrec_port_binding_set_chassis(pb, NULL);
>>      }
>> -    sbrec_port_binding_set_chassis(pb, NULL);
>>  
>>      if (pb->virtual_parent) {
>> +        if (cant_update_sb) {
>> +            return false;
>> +        }
>>          sbrec_port_binding_set_virtual_parent(pb, NULL);
>>      }
>> +
>> +    return true;
>>  }
>>  
>> -static void
>> +static bool
>> +release_local_binding_children(struct local_binding *lbinding,
>> +                               bool cant_update_sb)
>> +{
>> +    struct local_binding *l;
>> +    LIST_FOR_EACH (l, node, &lbinding->children) {
>> +        if (!release_lport(l->pb, cant_update_sb)) {
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool
>> +release_local_binding(struct local_binding *lbinding, bool cant_update_sb)
>> +{
>> +    if (!release_local_binding_children(lbinding, cant_update_sb)) {
>> +        return false;
>> +    }
>> +
>> +    if (!release_lport(lbinding->pb, cant_update_sb)) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool
>>  consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
>>                                struct binding_ctx_in *b_ctx_in,
>>                                enum local_binding_type binding_type,
>> @@ -644,6 +724,12 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
>>                                struct binding_ctx_out *b_ctx_out,
>>                                struct hmap *qos_map)
>>  {
>> +    if (pb->type[0] && strcmp(pb->type, "virtual")) {
>> +        /* The port binding type should be empty or 'virtual'
>> +         * to consider it for port binding here. */
>> +        return true;
>> +    }
> 
> In binding_run() we do the check outside consider_port_binding_for_vif()
> but in binding_handle_ovs_interface_changes() we
> consider_port_binding_for_vif() check the PB type. It would be nice to
> decide on one of the two ways.
> 
>> +
>>      const char *vif_chassis = smap_get(&pb->options, "requested-chassis");
>>      bool can_bind = !vif_chassis || !vif_chassis[0]
>>                      || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name)
>> @@ -660,11 +746,14 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
>>                       "Virtual port %s should not be bound to OVS port %s",
>>                       pb->logical_port, lbinding->iface->name);
>>          lbinding->pb = NULL;
>> -        return;
>> +        return false;
>>      }
>>  
>>      if (lbinding && lbinding->pb && can_bind) {
>> -        claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface);
>> +        if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface,
>> +                         !b_ctx_in->ovnsb_idl_txn)) {
>> +            return false;
>> +        }
>>  
>>          switch (binding_type) {
>>          case BT_VIF:
>> @@ -712,22 +801,26 @@ consider_port_binding_for_vif(const struct sbrec_port_binding *pb,
>>  
>>      if (pb->chassis == b_ctx_in->chassis_rec) {
>>          if (!lbinding || !lbinding->pb || !can_bind) {
>> -            release_lport(pb);
>> +            if (!release_lport(pb, !b_ctx_in->ovnsb_idl_txn)) {
>> +                return false;
>> +            }

We could just "return release_lport(pb, !b_ctx_in->ovnsb_idl_txn);" here
instead.

>>          }
>>      }
>> +
>> +    return true;
>>  }
>>  
>> -static void
>> +static bool
>>  consider_port_binding(const struct sbrec_port_binding *pb,
>>                        struct binding_ctx_in *b_ctx_in,
>>                        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,
>>                                        b_ctx_out->local_lports);
>>  
>> +    bool success = true;
> 
> We don't really need the "success" variable, we can just return
> claim_lport()/release_lport() below or false at the end of the function.
> 
>>      if (!strcmp(pb->type, "l2gateway")) {
>>          if (our_chassis) {
>>              sset_add(b_ctx_out->local_lports, pb->logical_port);
>> @@ -775,12 +868,14 @@ consider_port_binding(const struct sbrec_port_binding *pb,
>>          update_local_lport_ids(b_ctx_out->local_lport_ids, pb);
>>      }
>>  
>> -    ovs_assert(b_ctx_in->ovnsb_idl_txn);
>>      if (our_chassis) {
>> -        claim_lport(pb, b_ctx_in->chassis_rec, NULL);
>> +        success = claim_lport(pb, b_ctx_in->chassis_rec, NULL,
>> +                              !b_ctx_in->ovnsb_idl_txn);
>>      } else if (pb->chassis == b_ctx_in->chassis_rec) {
>> -        release_lport(pb);
>> +        success = release_lport(pb, !b_ctx_in->ovnsb_idl_txn);
>>      }
>> +
>> +    return success;
>>  }
>>  
>>  static void
>> @@ -813,6 +908,8 @@ build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in,
>>                                                      BT_VIF);
>>                  local_binding_add(b_ctx_out->local_bindings, lbinding);
>>                  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. */
>> @@ -921,60 +1018,6 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out)
>>      hmap_destroy(&qos_map);
> 
> Unrelated to your change but I think hmap_destroy(&qos_map) above is not
> enough at it will not free the memory allocated for the nodes.
> 
>>  }
>>  
>> -/* 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->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
>> @@ -1009,3 +1052,390 @@ 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;
>> +    }
>> +

It's probably a bit more efficient and a bit more readable to search for
the port binding (i.e., compute "present") here instead of the beginning
of the function.

>> +    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;

Nit: s/i =0/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--;
>> +

Should we consider reallocating the peer_ports array if n_peer_ports <
n_allocated_peer_ports / 2 ?

Regards,
Dumitru

>> +    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,
>> +                                     bool *changed)
>> +{
>> +    if (!b_ctx_in->chassis_rec) {
>> +        return false;
>> +    }
>> +
>> +    bool handled = true;
>> +    *changed = 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 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] &&
>> +            strcmp(iface_rec->type, "internal")) {
>> +            /* Right now are not handling ovs_interface changes of
>> +             * other types. This can be enhanced to handle of
>> +             * types - patch and tunnel. */
>> +            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) {
>> +
>> +                if (!release_local_binding(lbinding,
>> +                                           !b_ctx_in->ovnsb_idl_txn)) {
>> +                    handled = false;
>> +                    goto out;
>> +                }
>> +                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);
>> +                *changed = true;
>> +            }
>> +
>> +            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) {
>> +            *changed = true;
>> +            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 &&
>> +                    !strcmp(claim_lbinding->pb->type, "virtual")) {
>> +                    claim_lbinding->pb = NULL;
>> +                }
>> +            }
>> +
>> +            if (claim_lbinding->pb) {
>> +                if (!consider_port_binding_for_vif(claim_lbinding->pb,
>> +                                                   b_ctx_in, BT_VIF,
>> +                                                   claim_lbinding,
>> +                                                   b_ctx_out, qos_map_ptr)) {
>> +                    handled = false;
>> +                    goto out;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    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:
> 
> I think hmap_destroy(&qos_map) is not enough because it will not free
> the nodes in the qos_map, if any. Also, the "out" label should be before
> cleaning up the qos_map otherwise we'll leak memory when
> consider_port_binding_for_vif() returns false.
> 
>> +    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 *changed)
>> +{
>> +    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;
>> +
>> +    *changed = false;
>> +
>> +    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) {
>> +                lbinding->pb = NULL;
>> +                if (binding_type == BT_VIF &&
>> +                    !release_local_binding_children(
>> +                        lbinding, !b_ctx_in->ovnsb_idl_txn)) {
>> +                    handled = false;
>> +                    break;
>> +                }
>> +                *changed = true;
>> +            }
>> +
>> +            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) {
>> +                    lbinding->pb = pb;
>> +                    bool claimed = (pb->chassis == b_ctx_in->chassis_rec);
>> +                    if (!consider_port_binding_for_vif(
>> +                            pb, b_ctx_in, binding_type, lbinding, b_ctx_out,
>> +                            qos_map_ptr)) {
>> +                        handled = false;
>> +                        break;
>> +                    }
>> +                    bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec);
>> +                    if (!claimed && now_claimed) {
>> +                        *changed = true;
>> +                    }
>> +                }
>> +            } else {
>> +                if (!consider_port_binding(pb, b_ctx_in, b_ctx_out,
>> +                                           qos_map_ptr)) {
>> +                    handled = false;
>> +                    break;
>> +                }
>> +                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);
>> +                }
>> +                *changed = 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);
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    return handled;
>> +}
>> diff --git a/controller/binding.h b/controller/binding.h
>> index 6bee1d12e..fda680723 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,14 @@ 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 *changed);
>> +bool binding_handle_port_binding_changes(struct binding_ctx_in *,
>> +                                         struct binding_ctx_out *,
>> +                                         bool *changed);
>>  #endif /* controller/binding.h */
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 36a1cadb9..37ffc399c 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
>> @@ -1111,11 +1121,13 @@ 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);
>>          shash_init(&rt_data->local_bindings);
>> +        smap_init(&rt_data->local_iface_ids);
>>      }
>>  
>>      struct binding_ctx_in b_ctx_in;
>> @@ -1140,6 +1152,34 @@ en_runtime_data_run(struct engine_node *node, void *data)
>>      engine_set_node_state(node, EN_UPDATED);
>>  }
>>  
>> +static bool
>> +runtime_data_ovs_interface_handler(struct engine_node *node, void *data)
>> +{
>> +    struct ed_type_runtime_data *rt_data = data;
>> +    struct binding_ctx_in b_ctx_in;
>> +    struct binding_ctx_out b_ctx_out;
>> +    init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
>> +
>> +    bool changed = false;
>> +    if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out,
>> +                                              &changed)) {
>> +        return false;
>> +    }
>> +
>> +    if (changed) {
>> +        engine_set_node_state(node, EN_UPDATED);
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool
>> +runtime_data_noop_handler(struct engine_node *node OVS_UNUSED,
>> +                          void *data OVS_UNUSED)
>> +{
>> +    return true;
>> +}
>> +
>>  static bool
>>  runtime_data_sb_port_binding_handler(struct engine_node *node, void *data)
>>  {
>> @@ -1147,11 +1187,21 @@ runtime_data_sb_port_binding_handler(struct engine_node *node, void *data)
>>      struct binding_ctx_in b_ctx_in;
>>      struct binding_ctx_out b_ctx_out;
>>      init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
>> +    if (!b_ctx_in.chassis_rec) {
>> +        return false;
>> +    }
>> +
>> +    bool changed = false;
>> +    if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out,
>> +                                             &changed)) {
>> +        return false;
>> +    }
>>  
>> -    bool changed = binding_evaluate_port_binding_changes(&b_ctx_in,
>> -                                                         &b_ctx_out);
>> +    if (changed) {
>> +        engine_set_node_state(node, EN_UPDATED);
>> +    }
>>  
>> -    return !changed;
>> +    return true;
>>  }
>>  
>>  /* Connection tracking zones. */
>> @@ -1893,7 +1943,10 @@ main(int argc, char *argv[])
>>  
>>      engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, NULL);
>>      engine_add_input(&en_runtime_data, &en_ovs_bridge, NULL);
>> -    engine_add_input(&en_runtime_data, &en_ovs_port, NULL);
>> +    engine_add_input(&en_runtime_data, &en_ovs_port,
>> +                     runtime_data_noop_handler);
>> +    engine_add_input(&en_runtime_data, &en_ovs_interface,
>> +                     runtime_data_ovs_interface_handler);
>>      engine_add_input(&en_runtime_data, &en_ovs_qos, NULL);
>>  
>>      engine_add_input(&en_runtime_data, &en_sb_chassis, NULL);
>> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
>> index a8a15f8fe..5dfc6f7ca 100644
>> --- a/tests/ovn-performance.at
>> +++ b/tests/ovn-performance.at
>> @@ -239,6 +239,19 @@ for i in 1 2; do
>>      ovn_attach n1 br-phys 192.168.0.$i
>>  done
>>  
>> +# Wait for the tunnel ports to be created and up.
>> +# Otherwise this may affect the lflow_run count.
>> +
>> +OVS_WAIT_UNTIL([
>> +    test $(as hv1 ovs-vsctl list interface ovn-hv2-0 | \
>> +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1
>> +])
>> +
>> +OVS_WAIT_UNTIL([
>> +    test $(as hv2 ovs-vsctl list interface ovn-hv1-0 | \
>> +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1
>> +])
>> +
>>  # Add router lr1
>>  OVN_CONTROLLER_EXPECT_HIT(
>>      [hv1 hv2], [lflow_run],
>>
> 



More information about the dev mailing list