On Mon, May 11, 2020 at 7:57 PM Dumitru Ceara <dce...@redhat.com> wrote:
> On 5/11/20 11:45 AM, num...@ovn.org wrote: > > From: Numan Siddique <num...@ovn.org> > > > > This patch adds a new data structure - 'struct local_binding' which > represents > > a probable local port binding. A new object of this structure is creared > for > > each interface present in the integration bridge (br-int) with the > > external_ids:iface-id set. This struct has 2 main fields > > - 'struct ovsrec_interface *' and 'struct sbrec_port_binding *'. These > fields > > are updated during port claim and release. > > > > A shash of the local bindings is maintained with the 'iface-id' as the > hash key > > in the runtime data of the incremental processing engine. > > Hi Numan, > > Thanks Dumitru for the reviews. > What if there's a misconfiguration and two OVS interfaces on the same > hypervisor have the same iface-id? Isn't it better and more efficient to > use the interface ofport as key in a hmap instead? > > I think this is wrong configuration from the userside. If this happens, then we should have only one instance of 'struct local_binding' and ovn-controller can bind either of the port_Binding's. With the present master, ovn-controller binds only one of port_Binding. With iface-id as key, when any port binding changes happen we can easily look into the 'local_bindings' shash using the port_Binding->logical_port. So I'd stick with using the iface-id as the key. The version v4 of this patch would have handled this use case properly as the function - build_local_bindings_from_local_ifaces() was looking for 'iface-id' in the local_bindings before creating it.This version doesn't do it and I'll fix it in v6. Thanks. More comments inline. > > Thanks, > Dumitru > > > > > This patch helps in the upcoming patch to add incremental processing > support > > for OVS interface and SB port binding changes. > > > > Signed-off-by: Numan Siddique <num...@ovn.org> > > --- > > controller/binding.c | 696 +++++++++++++++++++++--------------- > > controller/binding.h | 16 +- > > controller/ovn-controller.c | 48 ++- > > 3 files changed, 439 insertions(+), 321 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 9d37a23cc..e35440c78 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -69,47 +69,6 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type); > > } > > > > -static void > > -get_local_iface_ids(const struct ovsrec_bridge *br_int, > > - struct shash *lport_to_iface, > > - struct sset *local_lports, > > - struct sset *egress_ifaces) > > -{ > > - int i; > > - > > - for (i = 0; i < br_int->n_ports; i++) { > > - const struct ovsrec_port *port_rec = br_int->ports[i]; > > - const char *iface_id; > > - int j; > > - > > - if (!strcmp(port_rec->name, br_int->name)) { > > - continue; > > - } > > - > > - for (j = 0; j < port_rec->n_interfaces; j++) { > > - const struct ovsrec_interface *iface_rec; > > - > > - iface_rec = port_rec->interfaces[j]; > > - iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > > - int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : > 0; > > - > > - if (iface_id && ofport > 0) { > > - shash_add(lport_to_iface, iface_id, iface_rec); > > - sset_add(local_lports, iface_id); > > - } > > - > > - /* Check if this is a tunnel interface. */ > > - if (smap_get(&iface_rec->options, "remote_ip")) { > > - const char *tunnel_iface > > - = smap_get(&iface_rec->status, > "tunnel_egress_iface"); > > - if (tunnel_iface) { > > - sset_add(egress_ifaces, tunnel_iface); > > - } > > - } > > - } > > - } > > -} > > - > > static void > > add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > struct ovsdb_idl_index > *sbrec_port_binding_by_datapath, > > @@ -441,25 +400,11 @@ static bool > > is_our_chassis(const struct sbrec_chassis *chassis_rec, > > const struct sbrec_port_binding *binding_rec, > > const struct sset *active_tunnels, > > - const struct ovsrec_interface *iface_rec, > > const struct sset *local_lports) > > { > > - /* Ports of type "virtual" should never be explicitly bound to an > OVS > > - * port in the integration bridge. If that's the case, ignore the > binding > > - * and log a warning. > > - */ > > - if (iface_rec && !strcmp(binding_rec->type, "virtual")) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > - VLOG_WARN_RL(&rl, > > - "Virtual port %s should not be bound to OVS port > %s", > > - binding_rec->logical_port, iface_rec->name); > > - return false; > > - } > > - > > bool our_chassis = false; > > - if (iface_rec > > - || (binding_rec->parent_port && binding_rec->parent_port[0] && > > - sset_contains(local_lports, binding_rec->parent_port))) { > > + if (binding_rec->parent_port && binding_rec->parent_port[0] && > > + sset_contains(local_lports, binding_rec->parent_port)) { > > /* This port is in our chassis unless it is a localport. */ > > our_chassis = strcmp(binding_rec->type, "localport"); > > } else if (!strcmp(binding_rec->type, "l2gateway")) { > > @@ -481,175 +426,6 @@ is_our_chassis(const struct sbrec_chassis > *chassis_rec, > > return our_chassis; > > } > > > > -/* Updates 'binding_rec' and if the port binding is local also updates > the > > - * local datapaths and ports. > > - * Updates and returns the array of local virtual ports that will > require > > - * additional processing. > > - */ > > -static const struct sbrec_port_binding ** > > -consider_local_datapath(const struct sbrec_port_binding *binding_rec, > > - const struct ovsrec_interface *iface_rec, > > - struct binding_ctx_in *b_ctx_in, > > - struct binding_ctx_out *b_ctx_out, > > - struct hmap *qos_map, > > - const struct sbrec_port_binding **vpbs, > > - size_t *n_vpbs, size_t *n_allocated_vpbs) > > -{ > > - bool our_chassis = is_our_chassis(b_ctx_in->chassis_rec, > binding_rec, > > - b_ctx_in->active_tunnels, > iface_rec, > > - b_ctx_out->local_lports); > > - if (iface_rec > > - || (binding_rec->parent_port && binding_rec->parent_port[0] && > > - sset_contains(b_ctx_out->local_lports, > > - binding_rec->parent_port))) { > > - if (binding_rec->parent_port && binding_rec->parent_port[0]) { > > - /* Add child logical port to the set of all local ports. */ > > - sset_add(b_ctx_out->local_lports, > binding_rec->logical_port); > > - } > > - 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, > > - binding_rec->datapath, false, > > - b_ctx_out->local_datapaths); > > - if (iface_rec && qos_map && b_ctx_in->ovs_idl_txn) { > > - get_qos_params(binding_rec, qos_map); > > - } > > - } else if (!strcmp(binding_rec->type, "l2gateway")) { > > - if (our_chassis) { > > - sset_add(b_ctx_out->local_lports, > binding_rec->logical_port); > > - 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, > > - binding_rec->datapath, false, > > - b_ctx_out->local_datapaths); > > - } > > - } else if (!strcmp(binding_rec->type, "chassisredirect")) { > > - if (ha_chassis_group_contains(binding_rec->ha_chassis_group, > > - b_ctx_in->chassis_rec)) { > > - 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, > > - binding_rec->datapath, false, > > - b_ctx_out->local_datapaths); > > - } > > - } else if (!strcmp(binding_rec->type, "l3gateway")) { > > - if (our_chassis) { > > - 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, > > - binding_rec->datapath, true, > > - b_ctx_out->local_datapaths); > > - } > > - } else if (!strcmp(binding_rec->type, "localnet")) { > > - /* Add all localnet ports to local_lports so that we allocate > ct zones > > - * for them. */ > > - sset_add(b_ctx_out->local_lports, binding_rec->logical_port); > > - if (qos_map && b_ctx_in->ovs_idl_txn) { > > - get_qos_params(binding_rec, qos_map); > > - } > > - } else if (!strcmp(binding_rec->type, "external")) { > > - if (ha_chassis_group_contains(binding_rec->ha_chassis_group, > > - b_ctx_in->chassis_rec)) { > > - 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, > > - binding_rec->datapath, false, > > - b_ctx_out->local_datapaths); > > - } > > - } > > - > > - if (our_chassis > > - || !strcmp(binding_rec->type, "patch") > > - || !strcmp(binding_rec->type, "localport") > > - || !strcmp(binding_rec->type, "vtep") > > - || !strcmp(binding_rec->type, "localnet")) { > > - update_local_lport_ids(b_ctx_out->local_lport_ids, binding_rec); > > - } > > - > > - ovs_assert(b_ctx_in->ovnsb_idl_txn); > > - const char *vif_chassis = smap_get(&binding_rec->options, > > - "requested-chassis"); > > - bool can_bind = !vif_chassis || !vif_chassis[0] > > - || !strcmp(vif_chassis, b_ctx_in->chassis_rec->name) > > - || !strcmp(vif_chassis, > b_ctx_in->chassis_rec->hostname); > > - > > - if (can_bind && our_chassis) { > > - if (binding_rec->chassis != b_ctx_in->chassis_rec) { > > - if (binding_rec->chassis) { > > - VLOG_INFO("Changing chassis for lport %s from %s to > %s.", > > - binding_rec->logical_port, > > - binding_rec->chassis->name, > > - b_ctx_in->chassis_rec->name); > > - } else { > > - VLOG_INFO("Claiming lport %s for this chassis.", > > - binding_rec->logical_port); > > - } > > - for (int i = 0; i < binding_rec->n_mac; i++) { > > - VLOG_INFO("%s: Claiming %s", > > - binding_rec->logical_port, > binding_rec->mac[i]); > > - } > > - sbrec_port_binding_set_chassis(binding_rec, > b_ctx_in->chassis_rec); > > - } > > - /* Check if the port encap binding, if any, has changed */ > > - struct sbrec_encap *encap_rec = > > - sbrec_get_port_encap(b_ctx_in->chassis_rec, iface_rec); > > - if (encap_rec && binding_rec->encap != encap_rec) { > > - sbrec_port_binding_set_encap(binding_rec, encap_rec); > > - } > > - } else if (binding_rec->chassis == b_ctx_in->chassis_rec) { > > - if (!strcmp(binding_rec->type, "virtual")) { > > - if (*n_vpbs == *n_allocated_vpbs) { > > - vpbs = x2nrealloc(vpbs, n_allocated_vpbs, sizeof *vpbs); > > - } > > - vpbs[(*n_vpbs)] = binding_rec; > > - (*n_vpbs)++; > > - } else { > > - VLOG_INFO("Releasing lport %s from this chassis.", > > - binding_rec->logical_port); > > - if (binding_rec->encap) { > > - sbrec_port_binding_set_encap(binding_rec, NULL); > > - } > > - sbrec_port_binding_set_chassis(binding_rec, NULL); > > - } > > - } else if (our_chassis) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > - VLOG_INFO_RL(&rl, > > - "Not claiming lport %s, chassis %s " > > - "requested-chassis %s", > > - binding_rec->logical_port, > b_ctx_in->chassis_rec->name, > > - vif_chassis); > > - } > > - > > - return vpbs; > > -} > > - > > -static void > > -consider_local_virtual_port(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > - const struct sbrec_chassis *chassis_rec, > > - const struct sbrec_port_binding > *binding_rec) > > -{ > > - if (binding_rec->virtual_parent) { > > - const struct sbrec_port_binding *parent = > > - lport_lookup_by_name(sbrec_port_binding_by_name, > > - binding_rec->virtual_parent); > > - if (parent && parent->chassis == chassis_rec) { > > - return; > > - } > > - } > > - > > - /* pinctrl module takes care of binding the ports of type 'virtual'. > > - * Release such ports if their virtual parents are no longer > claimed by > > - * this chassis. > > - */ > > - VLOG_INFO("Releasing lport %s from this chassis.", > > - binding_rec->logical_port); > > - if (binding_rec->encap) { > > - sbrec_port_binding_set_encap(binding_rec, NULL); > > - } > > - sbrec_port_binding_set_chassis(binding_rec, NULL); > > - sbrec_port_binding_set_virtual_parent(binding_rec, NULL); > > -} > > - > > static void > > add_localnet_egress_interface_mappings( > > const struct sbrec_port_binding *port_binding, > > @@ -712,6 +488,345 @@ consider_localnet_port(const struct > sbrec_port_binding *binding_rec, > > ld->localnet_port = binding_rec; > > } > > > > +enum local_binding_type { > > + BT_VIF, > > + BT_CHILD, > > + BT_VIRTUAL > > +}; > > I think a comment about how local_binding structures are used and their > hierarchy would be really useful. It might make the code easier to read > if we describe the different local_binding_type values and how they're > used here. > Ack. I'll address it in v6. > > > + > > +struct local_binding { > > + struct ovs_list node; /* In parent if any. */ > > + char *name; > > + enum local_binding_type type; > > + const struct ovsrec_interface *iface; > > + const struct sbrec_port_binding *pb; > > + > > + struct ovs_list children; > > +}; > > + > > +static struct local_binding * > > +local_binding_create(const char *name, const struct ovsrec_interface > *iface, > > + const struct sbrec_port_binding *pb, > > + enum local_binding_type type) > > +{ > > + struct local_binding *lbinding = xzalloc(sizeof *lbinding); > > + lbinding->name = xstrdup(name); > > + lbinding->type = type; > > + lbinding->pb = pb; > > + lbinding->iface = iface; > > + ovs_list_init(&lbinding->children); > > + return lbinding; > > +} > > + > > +static void > > +local_binding_add(struct shash *local_bindings, struct local_binding > *lbinding) > > +{ > > + shash_add(local_bindings, lbinding->name, lbinding); > > +} > > + > > +static struct local_binding * > > +local_binding_find(struct shash *local_bindings, const char *name) > > +{ > > + return shash_find_data(local_bindings, name); > > +} > > + > > +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); > > + } > > Here we leak the lbinding->name memory. > > > + } > > + > > + shash_destroy(local_bindings); > > Here we leak the node->data (i.e., struct local_binding) memory. We > should probably use shash_destroy_free_data(). > I somehow assumed that shash_destroy will also free the userdata. I'll fix it in v6. > > > +} > > + > > +static void > > +local_binding_add_child(struct local_binding *lbinding, > > + struct local_binding *child) > > +{ > > + struct local_binding *l; > > + LIST_FOR_EACH (l, node, &lbinding->children) { > > + if (l == child) { > > + return; > > + } > > + } > > Shouldn't we use local_binding_find_child() first before adding a child > instead? Also, it seems a bit odd that we have to check that the pointer > is not already in the list. > > Actually the caller of this function, first calls local_binding_find_child() before calling this function. So I'd just remove the for loop. Another thing is, do we really want to walk lists of children or should > we use a shash/hmap? It might make sense to stick with list if we don't > expect a lot of child nodes though. > It can happen that a port_Binding can be a parent for many other port_bindings in the case of a kubernetes deployed on top of openstack using Kuryr. I'll think further and see if we can have a shash. > > + > > + ovs_list_push_back(&lbinding->children, &child->node); > > +} > > + > > +static struct local_binding * > > +local_binding_find_child(struct local_binding *lbinding, > > + const char *child_name) > > +{ > > + struct local_binding *l; > > + LIST_FOR_EACH (l, node, &lbinding->children) { > > + if (!strcmp(l->name, child_name)) { > > + return l; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +void > > +binding_add_vport_to_local_bindings(struct shash *local_bindings, > > + const struct sbrec_port_binding > *parent, > > + const struct sbrec_port_binding > *vport) > > This function is never called anywhere as far as I see. And if I > understand correctly this should be called when incrementally processing > new port bindings of type "virtual". Does it mean that we just lose the > update in that case? > > Actually this is a stale function which is no longer used. It was used in earlier versions. I forgot to clean this up. I'll clean it in v6. > > +{ > > + struct local_binding *lbinding = local_binding_find(local_bindings, > > + > parent->logical_port); > > + ovs_assert(lbinding); > > + struct local_binding *vbinding = > > + local_binding_find_child(lbinding, vport->logical_port); > > + if (!vbinding) { > > + vbinding = local_binding_create(vport->logical_port, > lbinding->iface, > > + vport, BT_VIRTUAL); > > + local_binding_add_child(lbinding, vbinding); > > + } else { > > + vbinding->type = BT_VIRTUAL; > > + } > > +} > > + > > +static void > > +claim_lport(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec, > > + const struct ovsrec_interface *iface_rec) > > +{ > > + if (pb->chassis != chassis_rec) { > > + if (pb->chassis) { > > + VLOG_INFO("Changing chassis for lport %s from %s to %s.", > > + pb->logical_port, pb->chassis->name, > > + chassis_rec->name); > > + } else { > > + VLOG_INFO("Claiming lport %s for this chassis.", > pb->logical_port); > > + } > > + for (int i = 0; i < pb->n_mac; i++) { > > + VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); > > + } > > + > > + sbrec_port_binding_set_chassis(pb, chassis_rec); > > + } > > + > > + /* Check if the port encap binding, if any, has changed */ > > + struct sbrec_encap *encap_rec = > > + sbrec_get_port_encap(chassis_rec, iface_rec); > > + if (encap_rec && pb->encap != encap_rec) { > > + sbrec_port_binding_set_encap(pb, encap_rec); > > + } > > +} > > + > > +static void > > +release_lport(const struct sbrec_port_binding *pb) > > +{ > > + VLOG_INFO("Releasing lport %s from this chassis.", > pb->logical_port); > > + if (pb->encap) { > > + sbrec_port_binding_set_encap(pb, NULL); > > + } > > + sbrec_port_binding_set_chassis(pb, NULL); > > + > > + if (pb->virtual_parent) { > > + sbrec_port_binding_set_virtual_parent(pb, NULL); > > + } > > +} > > + > > +static void > > +consider_port_binding_for_vif(const struct sbrec_port_binding *pb, > > + struct binding_ctx_in *b_ctx_in, > > + enum local_binding_type binding_type, > > + struct local_binding *lbinding, > > + struct binding_ctx_out *b_ctx_out, > > + struct hmap *qos_map) > > +{ > > + 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) > > + || !strcmp(vif_chassis, > b_ctx_in->chassis_rec->hostname); > > + > > + /* Ports of type "virtual" should never be explicitly bound to an > OVS > > + * port in the integration bridge. If that's the case, ignore the > binding > > + * and log a warning. > > + */ > > + if (!strcmp(pb->type, "virtual") && lbinding && lbinding->iface && > > + binding_type == BT_VIF) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_WARN_RL(&rl, > > + "Virtual port %s should not be bound to OVS port > %s", > > + pb->logical_port, lbinding->iface->name); > > + lbinding->pb = NULL; > > + return; > > + } > > + > > + if (lbinding && lbinding->pb && can_bind) { > > + claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface); > > + > > + switch (binding_type) { > > + case BT_VIF: > > + lbinding->pb = pb; > > + break; > > + case BT_CHILD: > > + case BT_VIRTUAL: { > > + /* Add child logical port to the set of all local ports. */ > > + sset_add(b_ctx_out->local_lports, pb->logical_port); > > + struct local_binding *child = > > + local_binding_find_child(lbinding, pb->logical_port); > > + if (!child) { > > + child = local_binding_create(pb->logical_port, > lbinding->iface, > > + pb, binding_type); > > + local_binding_add_child(lbinding, child); > > + } else { > > + ovs_assert(child->type == BT_CHILD || > > + child->type == BT_VIRTUAL); > > + child->pb = pb; > > + child->iface = lbinding->iface; > > + } > > + break; > > + } > > + default: > > + OVS_NOT_REACHED(); > > + } > > + > > + 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, > > + pb->datapath, false, > b_ctx_out->local_datapaths); > > + update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > > + if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > + get_qos_params(pb, qos_map); > > + } > > + } else if (lbinding && lbinding->pb && !can_bind) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_INFO_RL(&rl, > > + "Not claiming lport %s, chassis %s " > > + "requested-chassis %s", > > + pb->logical_port, > > + b_ctx_in->chassis_rec->name, > > + vif_chassis); > > + } > > + > > + if (pb->chassis == b_ctx_in->chassis_rec) { > > + if (!lbinding || !lbinding->pb || !can_bind) { > > + release_lport(pb); > > + } > > + } > > +} > > + > > +static void > > +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); > > + > > + if (!strcmp(pb->type, "l2gateway")) { > > + if (our_chassis) { > > + sset_add(b_ctx_out->local_lports, pb->logical_port); > > + 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, > > + pb->datapath, false, > > + b_ctx_out->local_datapaths); > > + } > > + } else if (!strcmp(pb->type, "chassisredirect")) { > > + if (ha_chassis_group_contains(pb->ha_chassis_group, > > + b_ctx_in->chassis_rec)) { > > + 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, > > + pb->datapath, false, > > + b_ctx_out->local_datapaths); > > + } > > + } else if (!strcmp(pb->type, "l3gateway")) { > > + if (our_chassis) { > > + 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, > > + pb->datapath, true, > b_ctx_out->local_datapaths); > > + } > > + } else if (!strcmp(pb->type, "localnet")) { > > + /* Add all localnet ports to local_lports so that we allocate > ct zones > > + * for them. */ > > + sset_add(b_ctx_out->local_lports, pb->logical_port); > > + if (qos_map && b_ctx_in->ovs_idl_txn) { > > + get_qos_params(pb, qos_map); > > + } > > + } else if (!strcmp(pb->type, "external")) { > > + if (ha_chassis_group_contains(pb->ha_chassis_group, > > + b_ctx_in->chassis_rec)) { > > + 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, > > + pb->datapath, false, > > + b_ctx_out->local_datapaths); > > + } > > + } > > + > > + if (our_chassis || !strcmp(pb->type, "localnet")) { > > + 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); > > + } else if (pb->chassis == b_ctx_in->chassis_rec) { > > + release_lport(pb); > > + } > > +} > > + > > +static void > > +build_local_bindings_from_local_ifaces(struct binding_ctx_in *b_ctx_in, > > + struct binding_ctx_out > *b_ctx_out) > > +{ > > + int i; > > + for (i = 0; i < b_ctx_in->br_int->n_ports; i++) { > > + const struct ovsrec_port *port_rec = b_ctx_in->br_int->ports[i]; > > + const char *iface_id; > > + int j; > > + > > + if (!strcmp(port_rec->name, b_ctx_in->br_int->name)) { > > + continue; > > + } > > + > > + for (j = 0; j < port_rec->n_interfaces; j++) { > > + const struct ovsrec_interface *iface_rec; > > + > > + iface_rec = port_rec->interfaces[j]; > > + iface_id = smap_get(&iface_rec->external_ids, "iface-id"); > > + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : > 0; > > + > > + if (iface_id && ofport > 0) { > > + const struct sbrec_port_binding *pb > > + = lport_lookup_by_name( > > + b_ctx_in->sbrec_port_binding_by_name, iface_id); > > + struct local_binding *lbinding = > > + lbinding = local_binding_create(iface_id, > iface_rec, pb, > > + BT_VIF); > > + local_binding_add(b_ctx_out->local_bindings, lbinding); > > + sset_add(b_ctx_out->local_lports, iface_id); > > + } > > + > > + /* Check if this is a tunnel interface. */ > > + if (smap_get(&iface_rec->options, "remote_ip")) { > > + const char *tunnel_iface > > + = smap_get(&iface_rec->status, > "tunnel_egress_iface"); > > + if (tunnel_iface) { > > + sset_add(b_ctx_out->egress_ifaces, tunnel_iface); > > + } > > + } > > + } > > + } > > +} > > + > > void > > binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out > *b_ctx_out) > > { > > @@ -721,50 +836,62 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > > > > const struct sbrec_port_binding *binding_rec; > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); > > - struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); > > - struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); > > struct hmap qos_map; > > > > hmap_init(&qos_map); > > if (b_ctx_in->br_int) { > > - get_local_iface_ids(b_ctx_in->br_int, &lport_to_iface, > > - b_ctx_out->local_lports, &egress_ifaces); > > + build_local_bindings_from_local_ifaces(b_ctx_in, b_ctx_out); > > } > > > > - /* Array to store pointers to local virtual ports. It is populated > by > > - * consider_local_datapath. > > - */ > > - const struct sbrec_port_binding **vpbs = NULL; > > - size_t n_vpbs = 0; > > - size_t n_allocated_vpbs = 0; > > + struct hmap *qos_map_ptr = > > + !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL; > > > > /* Run through each binding record to see if it is resident on this > > * chassis and update the binding accordingly. This includes both > > - * directly connected logical ports and children of those ports. > > - * Virtual ports are just added to vpbs array and will be processed > > - * later. This is special case for virtual ports is needed in order > to > > - * make sure we update the virtual_parent port bindings first. > > + * directly connected logical ports and children of those ports > > + * (which also includes virtual ports). > > */ > > SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, > > b_ctx_in->port_binding_table) { > > - const struct ovsrec_interface *iface_rec > > - = shash_find_data(&lport_to_iface, > binding_rec->logical_port); > > - vpbs = > > - consider_local_datapath(binding_rec, > > - iface_rec, b_ctx_in, b_ctx_out, > > - sset_is_empty(&egress_ifaces) ? > NULL : > > - &qos_map, > > - vpbs, &n_vpbs, &n_allocated_vpbs); > > - } > > - > > - /* Now also update the virtual ports in case their parent ports were > > - * updated above. > > - */ > > - for (size_t i = 0; i < n_vpbs; i++) { > > - > consider_local_virtual_port(b_ctx_in->sbrec_port_binding_by_name, > > - b_ctx_in->chassis_rec, vpbs[i]); > > + if (!strcmp(binding_rec->type, "patch") || > > + !strcmp(binding_rec->type, "localport") || > > + !strcmp(binding_rec->type, "vtep")) { > > + update_local_lport_ids(b_ctx_out->local_lport_ids, > binding_rec); > > + continue; > > + } > > + > > + bool consider_for_vif = false; > > + struct local_binding *lbinding = NULL; > > + enum local_binding_type binding_type = BT_VIF; > > + 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); > > + binding_type = BT_CHILD; > > + } > > + > > + consider_for_vif = true; > > + } else if (!strcmp(binding_rec->type, "virtual") && > > + binding_rec->virtual_parent && > > + binding_rec->virtual_parent[0]) { > > + lbinding = local_binding_find(b_ctx_out->local_bindings, > > + binding_rec->virtual_parent); > > + consider_for_vif = true; > > + binding_type = BT_VIRTUAL; > > + } > > + > > + if (consider_for_vif) { > > + consider_port_binding_for_vif(binding_rec, b_ctx_in, > > + binding_type, lbinding, > b_ctx_out, > > + qos_map_ptr); > > + } else { > > + consider_port_binding(binding_rec, b_ctx_in, b_ctx_out, > > + qos_map_ptr); > > + } > > Is it maybe more readable if we have different port binding handlers > based on type? I.e., consider_port_binding_<TYPE>: > - consider_port_binding_vif() > - consider_port_binding_virtual() > - consider_port_binding_vport() > - consider_port_binding_l2gateway() > - consider_port_binding_chassisredirect() > - consider_port_binding_l3gateway() > - consider_port_binding_localnet() > - consider_port_binding_external() > > Right now we have two functions that split the work but do partially > different things for different types of port bindings. > Ok. I'll see if we can have this. > > Also, I think it might be a bit more readable if we'd define some helper > functions/macros to determine the type of a PB instead of combinations of: > - "!binding_rec->type[0]" > - "!strcmp(binding_rec->type, ...)" > > What do you think? > Sounds good. I'll address it in v6. > > > } > > - free(vpbs); > > > > add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, > > &bridge_mappings); > > @@ -776,49 +903,39 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > > b_ctx_in->port_binding_table) { > > if (!strcmp(binding_rec->type, "localnet")) { > > consider_localnet_port(binding_rec, &bridge_mappings, > > - &egress_ifaces, > b_ctx_out->local_datapaths); > > + b_ctx_out->egress_ifaces, > > + b_ctx_out->local_datapaths); > > } > > } > > shash_destroy(&bridge_mappings); > > > > - if (!sset_is_empty(&egress_ifaces) > > + if (!sset_is_empty(b_ctx_out->egress_ifaces) > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table, > > - b_ctx_in->qos_table, &egress_ifaces)) { > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) > { > > const char *entry; > > - SSET_FOR_EACH (entry, &egress_ifaces) { > > + SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > > setup_qos(entry, &qos_map); > > } > > } > > > > - shash_destroy(&lport_to_iface); > > - sset_destroy(&egress_ifaces); > > 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( > > - const struct sbrec_port_binding_table *pb_table, > > - const struct ovsrec_bridge *br_int, > > - const struct sbrec_chassis *chassis_rec, > > - struct sset *active_tunnels, > > - struct sset *local_lports) > > +binding_evaluate_port_binding_changes(struct binding_ctx_in *b_ctx_in, > > + struct binding_ctx_out *b_ctx_out) > > { > > - if (!chassis_rec) { > > + if (!b_ctx_in->chassis_rec) { > > return true; > > } > > > > bool changed = false; > > > > const struct sbrec_port_binding *binding_rec; > > - struct shash lport_to_iface = SHASH_INITIALIZER(&lport_to_iface); > > - struct sset egress_ifaces = SSET_INITIALIZER(&egress_ifaces); > > - if (br_int) { > > - get_local_iface_ids(br_int, &lport_to_iface, local_lports, > > - &egress_ifaces); > > - } > > - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, pb_table) { > > + 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 > > @@ -830,24 +947,31 @@ binding_evaluate_port_binding_changes( > > * interface table will be updated, which will trigger > recompute. > > * > > * - If the port is not a regular VIF, always trigger > recompute. */ > > - if (binding_rec->chassis == chassis_rec) { > > + if (binding_rec->chassis == b_ctx_in->chassis_rec) { > > changed = true; > > break; > > } > > > > - const struct ovsrec_interface *iface_rec > > - = shash_find_data(&lport_to_iface, > binding_rec->logical_port); > > - if (is_our_chassis(chassis_rec, binding_rec, active_tunnels, > iface_rec, > > - local_lports) || > > - (strcmp(binding_rec->type, "") && strcmp(binding_rec->type, > > - "remote"))) { > > + if (strcmp(binding_rec->type, "")) { > > Unless I'm missing something we lost the '&& strcmp(binding_rec->type, > "remote")' here. Is that desirable? > > Yes. It's desirable. For any port binding change of type other than normal PB (empty type) this function will return true. > > + 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; > > } > > } > > > > - shash_destroy(&lport_to_iface); > > - sset_destroy(&egress_ifaces); > > return changed; > > } > > > > diff --git a/controller/binding.h b/controller/binding.h > > index d0b8331af..6bee1d12e 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -31,6 +31,7 @@ struct ovsrec_open_vswitch_table; > > struct sbrec_chassis; > > struct sbrec_port_binding_table; > > struct sset; > > +struct sbrec_port_binding; > > > > struct binding_ctx_in { > > struct ovsdb_idl_txn *ovnsb_idl_txn; > > @@ -51,8 +52,10 @@ struct binding_ctx_in { > > > > struct binding_ctx_out { > > struct hmap *local_datapaths; > > + struct shash *local_bindings; > > struct sset *local_lports; > > struct sset *local_lport_ids; > > + struct sset *egress_ifaces; > > }; > > > > void binding_register_ovs_idl(struct ovsdb_idl *); > > @@ -60,11 +63,10 @@ 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( > > - const struct sbrec_port_binding_table *, > > - const struct ovsrec_bridge *br_int, > > - const struct sbrec_chassis *, > > - struct sset *active_tunnels, > > - struct sset *local_lports); > > - > > +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); > > #endif /* controller/binding.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index c0a97a265..36a1cadb9 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -958,6 +958,9 @@ struct ed_type_runtime_data { > > /* Contains "struct local_datapath" nodes. */ > > struct hmap local_datapaths; > > > > + /* Contains "struct local_bindings" nodes. */ > > s/"struct local_bindings"/"struct local_binding" > Ack. > > > + struct shash local_bindings; > > + > > /* Contains the name of each logical port resident on the local > > * hypervisor. These logical ports include the VIFs (and their > child > > * logical ports, if any) that belong to VMs running on the > hypervisor, > > @@ -969,6 +972,8 @@ struct ed_type_runtime_data { > > * <datapath-tunnel-key>_<port-tunnel-key> */ > > struct sset local_lport_ids; > > struct sset active_tunnels; > > + > > + struct sset egress_ifaces; > > }; > > > > static void * > > @@ -981,6 +986,8 @@ en_runtime_data_init(struct engine_node *node > OVS_UNUSED, > > sset_init(&data->local_lports); > > sset_init(&data->local_lport_ids); > > sset_init(&data->active_tunnels); > > + sset_init(&data->egress_ifaces); > > + shash_init(&data->local_bindings); > > We have a local_bindings_destroy() function that we call below, I think > it would be nice to have a local_bindings_init() function too. It seems > a bit weird that the only interface we expose for "struct local_binding" > is the "destroy" interface. > Is it fine even if it is empty ? or you want shash_init to be done in that function ? > > > return data; > > } > > > > @@ -992,6 +999,7 @@ en_runtime_data_cleanup(void *data) > > sset_destroy(&rt_data->local_lports); > > sset_destroy(&rt_data->local_lport_ids); > > sset_destroy(&rt_data->active_tunnels); > > + sset_init(&rt_data->egress_ifaces); > > struct local_datapath *cur_node, *next_node; > > HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, > > &rt_data->local_datapaths) { > > @@ -1000,6 +1008,7 @@ en_runtime_data_cleanup(void *data) > > free(cur_node); > > } > > hmap_destroy(&rt_data->local_datapaths); > > + local_bindings_destroy(&rt_data->local_bindings); > > } > > > > static void > > @@ -1072,6 +1081,8 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->local_datapaths = &rt_data->local_datapaths; > > b_ctx_out->local_lports = &rt_data->local_lports; > > 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; > > } > > > > static void > > @@ -1095,12 +1106,16 @@ en_runtime_data_run(struct engine_node *node, > void *data) > > free(cur_node); > > } > > hmap_clear(local_datapaths); > > + local_bindings_destroy(&rt_data->local_bindings); > > sset_destroy(local_lports); > > sset_destroy(local_lport_ids); > > sset_destroy(active_tunnels); > > + sset_destroy(&rt_data->egress_ifaces); > > 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); > > } > > > > struct binding_ctx_in b_ctx_in; > > @@ -1129,35 +1144,12 @@ static bool > > runtime_data_sb_port_binding_handler(struct engine_node *node, void > *data) > > { > > struct ed_type_runtime_data *rt_data = data; > > - struct sset *local_lports = &rt_data->local_lports; > > - struct sset *active_tunnels = &rt_data->active_tunnels; > > - > > - struct ovsrec_open_vswitch_table *ovs_table = > > - (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET( > > - engine_get_input("OVS_open_vswitch", node)); > > - struct ovsrec_bridge_table *bridge_table = > > - (struct ovsrec_bridge_table *)EN_OVSDB_GET( > > - engine_get_input("OVS_bridge", node)); > > - const char *chassis_id = chassis_get_id(); > > - const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > ovs_table); > > - > > - ovs_assert(br_int && chassis_id); > > - > > - struct ovsdb_idl_index *sbrec_chassis_by_name = > > - engine_ovsdb_node_get_index( > > - engine_get_input("SB_chassis", node), > > - "name"); > > - > > - const struct sbrec_chassis *chassis > > - = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); > > - ovs_assert(chassis); > > - > > - struct sbrec_port_binding_table *pb_table = > > - (struct sbrec_port_binding_table *)EN_OVSDB_GET( > > - engine_get_input("SB_port_binding", node)); > > + 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( > > - pb_table, br_int, chassis, active_tunnels, local_lports); > > + bool changed = binding_evaluate_port_binding_changes(&b_ctx_in, > > + &b_ctx_out); > > > > return !changed; > > } > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev