On Sat, May 23, 2020 at 6:42 AM Han Zhou <hz...@ovn.org> wrote: > On Fri, May 22, 2020 at 3:07 PM Numan Siddique <num...@ovn.org> wrote: > > > > > > > > On Sat, May 23, 2020 at 3:10 AM Han Zhou <hz...@ovn.org> wrote: > >> > >> On Fri, May 22, 2020 at 1:51 AM Numan Siddique <num...@ovn.org> wrote: > >> > > >> > > >> > > >> > On Fri, May 22, 2020 at 1:17 AM Han Zhou <hz...@ovn.org> wrote: > >> >> > >> >> On Thu, May 21, 2020 at 3:49 AM Numan Siddique <num...@ovn.org> > wrote: > >> >> > > >> >> > > >> >> > > >> >> > On Thu, May 21, 2020 at 6:57 AM Han Zhou <hz...@ovn.org> wrote: > >> >> >> > >> >> >> Hi Numan, please see my comments below. > >> >> > > >> >> > > >> >> > Thanks for the review. Please see below for a few comments. > >> >> > > >> >> > > >> >> >> > >> >> >> > >> >> >> On Wed, May 20, 2020 at 12:41 PM <num...@ovn.org> wrote: > >> >> >> > > >> >> >> > From: Numan Siddique <num...@ovn.org> > >> >> >> > > >> >> >> > This patch handles SB port binding changes and OVS interface > changes > >> >> >> > incrementally in the runtime_data stage which otherwise would > have > >> >> >> > resulted in calls to binding_run(). > >> >> >> > > >> >> >> > Prior to this patch, port binding changes were handled > differently. > >> >> >> > The changes were only evaluated to see if they need full > recompute > >> >> >> > or they can be ignored. > >> >> >> > > >> >> >> > With this patch, the runtime data is updated with the changes > (both > >> >> >> > SB port binding and OVS interface) and ports are > claimed/released in > >> >> >> > the handlers itself, avoiding the need to trigger recompute of > >> runtime > >> >> >> > data stage. > >> >> >> > > >> >> >> > Acked-by: Mark Michelson <mmich...@redhat.com> > >> >> >> > Signed-off-by: Numan Siddique <num...@ovn.org> > >> >> >> > --- > >> >> >> > controller/binding.c | 906 > >> +++++++++++++++++++++++++++++++----- > >> >> >> > controller/binding.h | 9 +- > >> >> >> > controller/ovn-controller.c | 61 ++- > >> >> >> > tests/ovn-performance.at | 13 + > >> >> >> > 4 files changed, 855 insertions(+), 134 deletions(-) > >> >> >> > > >> >> >> > diff --git a/controller/binding.c b/controller/binding.c > >> >> >> > index 2997fcae8..d5e8e4773 100644 > >> >> >> > --- a/controller/binding.c > >> >> >> > +++ b/controller/binding.c > >> >> >> > @@ -360,17 +360,6 @@ destroy_qos_map(struct hmap *qos_map) > >> >> >> > hmap_destroy(qos_map); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > -update_local_lport_ids(struct sset *local_lport_ids, > >> >> >> > - const struct sbrec_port_binding > >> *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 > >> >> >> > @@ -449,10 +438,10 @@ is_network_plugged(const struct > >> >> sbrec_port_binding > >> >> >> *binding_rec, > >> >> >> > } > >> >> >> > > >> >> >> > static void > >> >> >> > -consider_localnet_port(const struct sbrec_port_binding > >> *binding_rec, > >> >> >> > - struct shash *bridge_mappings, > >> >> >> > - struct sset *egress_ifaces, > >> >> >> > - struct hmap *local_datapaths) > >> >> >> > +update_ld_localnet_port(const struct sbrec_port_binding > >> *binding_rec, > >> >> >> > + struct shash *bridge_mappings, > >> >> >> > + struct sset *egress_ifaces, > >> >> >> > + struct hmap *local_datapaths) > >> >> >> > { > >> >> >> > /* Ignore localnet ports for unplugged networks. */ > >> >> >> > if (!is_network_plugged(binding_rec, bridge_mappings)) { > >> >> >> > @@ -482,6 +471,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); > >> >> >> > +} > >> >> >> > + > >> >> >> > /* Local bindings. binding.c module binds the logical port > >> >> (represented > >> >> >> by > >> >> >> > * Port_Binding rows) and sets the 'chassis' column when it is > sees > >> >> the > >> >> >> > * OVS interface row (of type "" or "internal") with the > >> >> >> > @@ -599,6 +610,14 @@ local_bindings_destroy(struct shash > >> >> *local_bindings) > >> >> >> > shash_destroy(local_bindings); > >> >> >> > } > >> >> >> > > >> >> >> > +static > >> >> >> > +void local_binding_delete(struct shash *local_bindings, > >> >> >> > + struct local_binding *lbinding) > >> >> >> > +{ > >> >> >> > + shash_find_and_delete(local_bindings, lbinding->name); > >> >> >> > + local_binding_destroy(lbinding); > >> >> >> > +} > >> >> >> > + > >> >> >> > static void > >> >> >> > local_binding_add_child(struct local_binding *lbinding, > >> >> >> > struct local_binding *child) > >> >> >> > @@ -625,6 +644,7 @@ is_lport_container(const struct > >> sbrec_port_binding > >> >> >> *pb) > >> >> >> > return !pb->type[0] && pb->parent_port && > pb->parent_port[0]; > >> >> >> > } > >> >> >> > > >> >> >> > +/* Corresponds to each Port_Binding.type. */ > >> >> >> > enum en_lport_type { > >> >> >> > LP_UNKNOWN, > >> >> >> > LP_VIF, > >> >> >> > @@ -670,12 +690,20 @@ get_lport_type(const struct > sbrec_port_binding > >> >> *pb) > >> >> >> > return LP_UNKNOWN; > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +/* Returns false if lport is not claimed due to 'sb_readonly'. > >> >> >> > + * Returns true otherwise. > >> >> >> > + */ > >> >> >> > +static bool > >> >> >> > claim_lport(const struct sbrec_port_binding *pb, > >> >> >> > const struct sbrec_chassis *chassis_rec, > >> >> >> > - const struct ovsrec_interface *iface_rec) > >> >> >> > + const struct ovsrec_interface *iface_rec, > >> >> >> > + bool sb_readonly) > >> >> >> > { > >> >> >> > if (pb->chassis != chassis_rec) { > >> >> >> > + if (sb_readonly) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > if (pb->chassis) { > >> >> >> > VLOG_INFO("Changing chassis for lport %s from %s to > >> %s.", > >> >> >> > pb->logical_port, pb->chassis->name, > >> >> >> > @@ -694,26 +722,48 @@ claim_lport(const struct > sbrec_port_binding > >> *pb, > >> >> >> > struct sbrec_encap *encap_rec = > >> >> >> > sbrec_get_port_encap(chassis_rec, iface_rec); > >> >> >> > if (encap_rec && pb->encap != encap_rec) { > >> >> >> > + if (sb_readonly) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > sbrec_port_binding_set_encap(pb, encap_rec); > >> >> >> > } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > -release_lport(const struct sbrec_port_binding *pb) > >> >> >> > +/* Returns false if lport is not released due to 'sb_readonly'. > >> >> >> > + * Returns true otherwise. > >> >> >> > + */ > >> >> >> > +static bool > >> >> >> > +release_lport(const struct sbrec_port_binding *pb, bool > >> sb_readonly) > >> >> >> > { > >> >> >> > if (!pb) { > >> >> >> > - return; > >> >> >> > + return true; > >> >> >> > } > >> >> >> > > >> >> >> > - VLOG_INFO("Releasing lport %s from this chassis.", > >> >> pb->logical_port); > >> >> >> > if (pb->encap) { > >> >> >> > + if (sb_readonly) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > sbrec_port_binding_set_encap(pb, NULL); > >> >> >> > } > >> >> >> > - sbrec_port_binding_set_chassis(pb, NULL); > >> >> >> > + > >> >> >> > + if (pb->chassis) { > >> >> >> > + if (sb_readonly) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + sbrec_port_binding_set_chassis(pb, NULL); > >> >> >> > + } > >> >> >> > > >> >> >> > if (pb->virtual_parent) { > >> >> >> > + if (sb_readonly) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > sbrec_port_binding_set_virtual_parent(pb, NULL); > >> >> >> > } > >> >> >> > + > >> >> >> > + VLOG_INFO("Releasing lport %s from this chassis.", > >> >> pb->logical_port); > >> >> >> > + return true; > >> >> >> > } > >> >> >> > > >> >> >> > static bool > >> >> >> > @@ -738,7 +788,41 @@ can_bind_on_this_chassis(const struct > >> >> sbrec_chassis > >> >> >> *chassis_rec, > >> >> >> > || !strcmp(requested_chassis, > chassis_rec->hostname); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > +release_local_binding_children(const struct sbrec_chassis > >> >> *chassis_rec, > >> >> >> > + struct local_binding *lbinding, > >> >> >> > + bool sb_readonly) > >> >> >> > +{ > >> >> >> > + struct shash_node *node; > >> >> >> > + SHASH_FOR_EACH (node, &lbinding->children) { > >> >> >> > + struct local_binding *l = node->data; > >> >> >> > + if (is_lbinding_this_chassis(l, chassis_rec)) { > >> >> >> > + if (!release_lport(l->pb, sb_readonly)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +release_local_binding(const struct sbrec_chassis *chassis_rec, > >> >> >> > + struct local_binding *lbinding, bool > >> >> sb_readonly) > >> >> >> > +{ > >> >> >> > + if (!release_local_binding_children(chassis_rec, lbinding, > >> >> >> > + sb_readonly)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > >> >> >> > + return release_lport(lbinding->pb, sb_readonly); > >> >> >> > + } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > consider_vif_lport_(const struct sbrec_port_binding *pb, > >> >> >> > bool can_bind, const char *vif_chassis, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > @@ -750,7 +834,10 @@ consider_vif_lport_(const struct > >> >> sbrec_port_binding > >> >> >> *pb, > >> >> >> > if (lbinding_set) { > >> >> >> > if (can_bind) { > >> >> >> > /* We can claim the lport. */ > >> >> >> > - claim_lport(pb, b_ctx_in->chassis_rec, > >> lbinding->iface); > >> >> >> > + if (!claim_lport(pb, b_ctx_in->chassis_rec, > >> >> lbinding->iface, > >> >> >> > + !b_ctx_in->ovnsb_idl_txn)){ > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > > >> >> >> > > >> >> add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > >> >> >> > > >> b_ctx_in->sbrec_port_binding_by_datapath, > >> >> >> > @@ -775,13 +862,14 @@ consider_vif_lport_(const struct > >> >> sbrec_port_binding > >> >> >> *pb, > >> >> >> > if (pb->chassis == b_ctx_in->chassis_rec) { > >> >> >> > /* Release the lport if there is no lbinding. */ > >> >> >> > if (!lbinding_set || !can_bind) { > >> >> >> > - release_lport(pb); > >> >> >> > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > >> >> >> > } > >> >> >> > } > >> >> >> > > >> >> >> > + return true; > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_vif_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out, > >> >> >> > @@ -801,11 +889,11 @@ consider_vif_lport(const struct > >> >> sbrec_port_binding > >> >> >> *pb, > >> >> >> > lbinding->pb = pb; > >> >> >> > } > >> >> >> > > >> >> >> > - consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, > >> >> >> > - b_ctx_out, lbinding, qos_map); > >> >> >> > + return consider_vif_lport_(pb, can_bind, vif_chassis, > b_ctx_in, > >> >> >> > + b_ctx_out, lbinding, qos_map); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_container_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out, > >> >> >> > @@ -815,7 +903,39 @@ consider_container_lport(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > parent_lbinding = > local_binding_find(b_ctx_out->local_bindings, > >> >> >> > pb->parent_port); > >> >> >> > > >> >> >> > - if (parent_lbinding && !parent_lbinding->pb) { > >> >> >> > + if (!parent_lbinding) { > >> >> >> > + /* There is no local_binding for parent port. Create it > >> >> >> > + * without OVS interface row. This is the only > exception > >> >> >> > + * for creating the 'struct local_binding' object > without > >> >> >> > + * corresponding OVS interface row. > >> >> >> > + * > >> >> >> > + * This is required for the following reasons: > >> >> >> > + * - If a logical port P1 is created and then > >> >> >> > + * few container ports - C1, C2, .. are created > first > >> by > >> >> CMS. > >> >> >> > + * - And later when OVS interface row is created for > P1, > >> >> then > >> >> >> > + * we want the these container ports also be > claimed by > >> >> the > >> >> >> > + * chassis. > >> >> >> > + * */ > >> >> >> > >> >> >> It seems the assumption is, for all container PBs, their parent > PBs > >> >> should > >> >> >> always have local_binding created, no matter if they are bound to > >> current > >> >> >> chassis. > >> >> > > >> >> > > >> >> > That's right. > >> >> > > >> >> >> > >> >> >> However, at least some of the code in this patch would violate > this > >> >> >> assumption, e.g. handle_iface_delete() will call > >> local_binding_delete() > >> >> and > >> >> >> delete the parent binding and all the children container bindings. > >> >> > > >> >> > > >> >> > I missed this part. I'll handle it in next version. > >> >> > > >> >> >> > >> >> >> It is also better to document this clearly in the comment of > struct > >> >> >> local_binding for what should be expected in the local_bindings. > >> >> > > >> >> > > >> >> > I've added comments for the local_bindings struct. But it looks > like > >> it's > >> >> not > >> >> > sufficient. I'll add more . > >> >> > > >> >> >> My > >> >> >> understanding is, for each binding_run() or change_handler > execution, > >> the > >> >> >> expected items left in it is: > >> >> >> 1) For each interface that has iface-id configured. > >> >> > > >> >> > Yes. > >> >> > > >> >> >> > >> >> >> 2) For each container PBs and their parent PBs, no matter if they > are > >> >> bound > >> >> >> to this chassis. > >> >> > > >> >> > > >> >> > Yes. > >> >> > > >> >> >> > >> >> >> 3) For some of the non VIF PBs ... > >> >> > > >> >> > > >> >> > Only for the "virtual" PB if it's parent is bound. > >> >> > > >> >> >> > >> >> >> Is this correct? > >> >> >> > >> >> >> > + parent_lbinding = local_binding_create(pb->parent_port, > >> NULL, > >> >> >> NULL, > >> >> >> > + BT_VIF); > >> >> >> > + local_binding_add(b_ctx_out->local_bindings, > >> parent_lbinding); > >> >> >> > + } > >> >> >> > + > >> >> >> > + struct local_binding *container_lbinding = > >> >> >> > + local_binding_find_child(parent_lbinding, > >> pb->logical_port); > >> >> >> > + > >> >> >> > + if (!container_lbinding) { > >> >> >> > + container_lbinding = > local_binding_create(pb->logical_port, > >> >> >> > + > >> >> parent_lbinding->iface, > >> >> >> > + pb, > >> BT_CONTAINER); > >> >> >> > + local_binding_add_child(parent_lbinding, > >> container_lbinding); > >> >> >> > + } else { > >> >> >> > + ovs_assert(container_lbinding->type == BT_CONTAINER); > >> >> >> > + container_lbinding->pb = pb; > >> >> >> > + container_lbinding->iface = parent_lbinding->iface; > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (!parent_lbinding->pb) { > >> >> >> > parent_lbinding->pb = lport_lookup_by_name( > >> >> >> > b_ctx_in->sbrec_port_binding_by_name, > pb->parent_port); > >> >> >> > > >> >> >> > @@ -824,37 +944,28 @@ consider_container_lport(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > * So call consider_vif_lport() to process it > first. */ > >> >> >> > consider_vif_lport(parent_lbinding->pb, b_ctx_in, > >> >> b_ctx_out, > >> >> >> > parent_lbinding, qos_map); > >> >> >> > - } > >> >> >> > - } > >> >> >> > + } else { > >> >> >> > + /* The parent lport doesn't exist. Call > >> release_lport() to > >> >> >> > + * release the container lport, if it was bound > >> earlier. > >> >> */ > >> >> >> > + if (is_lbinding_this_chassis(container_lbinding, > >> >> >> > + > b_ctx_in->chassis_rec)) { > >> >> >> > + return release_lport(pb, > !b_ctx_in->ovnsb_idl_txn); > >> >> >> > + } > >> >> >> > > >> >> >> > - if (!parent_lbinding || !parent_lbinding->pb) { > >> >> >> > - /* Call release_lport, to release the container lport, > if > >> >> >> > - * it was bound earlier. */ > >> >> >> > - if (pb->chassis == b_ctx_in->chassis_rec) { > >> >> >> > - release_lport(pb); > >> >> >> > + return true; > >> >> >> > } > >> >> >> > - return; > >> >> >> > } > >> >> >> > > >> >> >> > - struct local_binding *container_lbinding = > >> >> >> > - local_binding_find_child(parent_lbinding, > >> pb->logical_port); > >> >> >> > - ovs_assert(!container_lbinding); > >> >> >> > - > >> >> >> > - container_lbinding = local_binding_create(pb->logical_port, > >> >> >> > - > >> parent_lbinding->iface, > >> >> >> > - pb, > BT_CONTAINER); > >> >> >> > - local_binding_add_child(parent_lbinding, > container_lbinding); > >> >> >> > - > >> >> >> > const char *vif_chassis = > >> smap_get(&parent_lbinding->pb->options, > >> >> >> > "requested-chassis"); > >> >> >> > bool can_bind = > can_bind_on_this_chassis(b_ctx_in->chassis_rec, > >> >> >> > vif_chassis); > >> >> >> > > >> >> >> > - consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, > >> >> b_ctx_out, > >> >> >> > - container_lbinding, qos_map); > >> >> >> > + return consider_vif_lport_(pb, can_bind, vif_chassis, > b_ctx_in, > >> >> >> b_ctx_out, > >> >> >> > + container_lbinding, qos_map); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_virtual_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out, > >> >> >> > @@ -881,11 +992,16 @@ consider_virtual_lport(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > if (is_lbinding_this_chassis(parent_lbinding, > >> >> >> b_ctx_in->chassis_rec)) { > >> >> >> > virtual_lbinding = > >> >> >> > local_binding_find_child(parent_lbinding, > >> >> pb->logical_port); > >> >> >> > - ovs_assert(!virtual_lbinding); > >> >> >> > - virtual_lbinding = > local_binding_create(pb->logical_port, > >> >> >> > - > >> >> parent_lbinding->iface, > >> >> >> > - pb, > BT_VIRTUAL); > >> >> >> > - local_binding_add_child(parent_lbinding, > virtual_lbinding); > >> >> >> > + if (!virtual_lbinding) { > >> >> >> > + virtual_lbinding = > >> local_binding_create(pb->logical_port, > >> >> >> > + > >> >> >> parent_lbinding->iface, > >> >> >> > + pb, > >> BT_VIRTUAL); > >> >> >> > + local_binding_add_child(parent_lbinding, > >> >> virtual_lbinding); > >> >> >> > + } else { > >> >> >> > + ovs_assert(virtual_lbinding->type == BT_VIRTUAL); > >> >> >> > + virtual_lbinding->pb = pb; > >> >> >> > + virtual_lbinding->iface = parent_lbinding->iface; > >> >> >> > + } > >> >> >> > } > >> >> >> > > >> >> >> > return consider_vif_lport_(pb, true, NULL, b_ctx_in, > b_ctx_out, > >> >> >> > @@ -895,14 +1011,13 @@ consider_virtual_lport(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > /* Considers either claiming the lport or releasing the lport > >> >> >> > * for non VIF lports. > >> >> >> > */ > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_nonvif_lport_(const struct sbrec_port_binding *pb, > >> >> >> > bool our_chassis, > >> >> >> > bool has_local_l3gateway, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out) > >> >> >> > { > >> >> >> > - ovs_assert(b_ctx_in->ovnsb_idl_txn); > >> >> >> > if (our_chassis) { > >> >> >> > sset_add(b_ctx_out->local_lports, pb->logical_port); > >> >> >> > > add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key, > >> >> >> > @@ -912,13 +1027,16 @@ consider_nonvif_lport_(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > b_ctx_out->local_datapaths); > >> >> >> > > >> >> >> > update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > >> >> >> > - claim_lport(pb, b_ctx_in->chassis_rec, NULL); > >> >> >> > + return claim_lport(pb, b_ctx_in->chassis_rec, NULL, > >> >> >> > + !b_ctx_in->ovnsb_idl_txn); > >> >> >> > } else if (pb->chassis == b_ctx_in->chassis_rec) { > >> >> >> > - release_lport(pb); > >> >> >> > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn); > >> >> >> > } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_l2gw_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out) > >> >> >> > @@ -927,10 +1045,10 @@ consider_l2gw_lport(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > bool our_chassis = chassis_id && !strcmp(chassis_id, > >> >> >> > > >> >> >> b_ctx_in->chassis_rec->name); > >> >> >> > > >> >> >> > - consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, > >> >> b_ctx_out); > >> >> >> > + return consider_nonvif_lport_(pb, our_chassis, false, > b_ctx_in, > >> >> >> b_ctx_out); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_l3gw_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out) > >> >> >> > @@ -939,7 +1057,7 @@ consider_l3gw_lport(const struct > >> >> sbrec_port_binding > >> >> >> *pb, > >> >> >> > bool our_chassis = chassis_id && !strcmp(chassis_id, > >> >> >> > > >> >> >> b_ctx_in->chassis_rec->name); > >> >> >> > > >> >> >> > - consider_nonvif_lport_(pb, our_chassis, true, b_ctx_in, > >> >> b_ctx_out); > >> >> >> > + return consider_nonvif_lport_(pb, our_chassis, true, > b_ctx_in, > >> >> >> b_ctx_out); > >> >> >> > } > >> >> >> > > >> >> >> > static void > >> >> >> > @@ -958,7 +1076,7 @@ consider_localnet_lport(const struct > >> >> >> sbrec_port_binding *pb, > >> >> >> > update_local_lport_ids(b_ctx_out->local_lport_ids, pb); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_ha_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out) > >> >> >> > @@ -986,18 +1104,18 @@ consider_ha_lport(const struct > >> >> sbrec_port_binding > >> >> >> *pb, > >> >> >> > b_ctx_out->local_datapaths); > >> >> >> > } > >> >> >> > > >> >> >> > - consider_nonvif_lport_(pb, our_chassis, false, b_ctx_in, > >> >> b_ctx_out); > >> >> >> > + return consider_nonvif_lport_(pb, our_chassis, false, > b_ctx_in, > >> >> >> b_ctx_out); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_cr_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out) > >> >> >> > { > >> >> >> > - consider_ha_lport(pb, b_ctx_in, b_ctx_out); > >> >> >> > + return consider_ha_lport(pb, b_ctx_in, b_ctx_out); > >> >> >> > } > >> >> >> > > >> >> >> > -static void > >> >> >> > +static bool > >> >> >> > consider_external_lport(const struct sbrec_port_binding *pb, > >> >> >> > struct binding_ctx_in *b_ctx_in, > >> >> >> > struct binding_ctx_out *b_ctx_out) > >> >> >> > @@ -1050,6 +1168,8 @@ build_local_bindings(struct binding_ctx_in > >> >> >> *b_ctx_in, > >> >> >> > } > >> >> >> > > >> >> >> > sset_add(b_ctx_out->local_lports, iface_id); > >> >> >> > + smap_replace(b_ctx_out->local_iface_ids, > >> >> iface_rec->name, > >> >> >> > + iface_id); > >> >> >> > } > >> >> >> > > >> >> >> > /* Check if this is a tunnel interface. */ > >> >> >> > @@ -1165,9 +1285,9 @@ binding_run(struct binding_ctx_in > *b_ctx_in, > >> >> struct > >> >> >> binding_ctx_out *b_ctx_out) > >> >> >> > * corresponding local datapath accordingly. */ > >> >> >> > struct localnet_lport *lnet_lport; > >> >> >> > LIST_FOR_EACH_POP (lnet_lport, list_node, &localnet_lports) > { > >> >> >> > - consider_localnet_port(lnet_lport->pb, > &bridge_mappings, > >> >> >> > - b_ctx_out->egress_ifaces, > >> >> >> > - b_ctx_out->local_datapaths); > >> >> >> > + update_ld_localnet_port(lnet_lport->pb, > &bridge_mappings, > >> >> >> > + b_ctx_out->egress_ifaces, > >> >> >> > + b_ctx_out->local_datapaths); > >> >> >> > free(lnet_lport); > >> >> >> > } > >> >> >> > > >> >> >> > @@ -1185,95 +1305,625 @@ binding_run(struct binding_ctx_in > >> *b_ctx_in, > >> >> >> struct binding_ctx_out *b_ctx_out) > >> >> >> > destroy_qos_map(&qos_map); > >> >> >> > } > >> >> >> > > >> >> >> > -/* Returns true if port-binding changes potentially require > flow > >> >> changes > >> >> >> on > >> >> >> > - * the current chassis. Returns false if we are sure there is > no > >> >> impact. > >> >> >> */ > >> >> >> > +/* Returns true if the database is all cleaned up, false if > more > >> work > >> >> is > >> >> >> > + * required. */ > >> >> >> > bool > >> >> >> > -binding_evaluate_port_binding_changes(struct binding_ctx_in > >> *b_ctx_in, > >> >> >> > - struct binding_ctx_out > >> >> *b_ctx_out) > >> >> >> > +binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > >> >> >> > + const struct sbrec_port_binding_table > >> >> >> *port_binding_table, > >> >> >> > + const struct sbrec_chassis *chassis_rec) > >> >> >> > { > >> >> >> > - if (!b_ctx_in->chassis_rec) { > >> >> >> > + if (!ovnsb_idl_txn) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + if (!chassis_rec) { > >> >> >> > return true; > >> >> >> > } > >> >> >> > > >> >> >> > - bool changed = false; > >> >> >> > - > >> >> >> > const struct sbrec_port_binding *binding_rec; > >> >> >> > - SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding_rec, > >> >> >> > - > >> >> >> b_ctx_in->port_binding_table) { > >> >> >> > - /* XXX: currently OVSDB change tracking doesn't support > >> >> getting > >> >> >> old > >> >> >> > - * data when the operation is update, so if a > port-binding > >> >> moved > >> >> >> from > >> >> >> > - * this chassis to another, there is no easy way to > find > >> out > >> >> the > >> >> >> > - * change. To workaround this problem, we just makes > sure > >> if > >> >> >> > - * any port *related to* this chassis has any change, > then > >> >> >> trigger > >> >> >> > - * recompute. > >> >> >> > - * > >> >> >> > - * - If a regular VIF is unbound from this chassis, the > >> local > >> >> >> ovsdb > >> >> >> > - * interface table will be updated, which will > trigger > >> >> >> recompute. > >> >> >> > - * > >> >> >> > - * - If the port is not a regular VIF, always trigger > >> >> recompute. > >> >> >> */ > >> >> >> > - if (binding_rec->chassis == b_ctx_in->chassis_rec) { > >> >> >> > - changed = true; > >> >> >> > + bool any_changes = false; > >> >> >> > + SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, > >> >> port_binding_table) { > >> >> >> > + if (binding_rec->chassis == chassis_rec) { > >> >> >> > + if (binding_rec->encap) { > >> >> >> > + sbrec_port_binding_set_encap(binding_rec, > NULL); > >> >> >> > + } > >> >> >> > + sbrec_port_binding_set_chassis(binding_rec, NULL); > >> >> >> > + any_changes = true; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (any_changes) { > >> >> >> > + ovsdb_idl_txn_add_comment( > >> >> >> > + ovnsb_idl_txn, > >> >> >> > + "ovn-controller: removing all port bindings for > '%s'", > >> >> >> > + chassis_rec->name); > >> >> >> > + } > >> >> >> > + > >> >> >> > + return !any_changes; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static void > >> >> >> > +add_local_datapath_peer_port(const struct sbrec_port_binding > *pb, > >> >> >> > + struct binding_ctx_in *b_ctx_in, > >> >> >> > + struct binding_ctx_out *b_ctx_out, > >> >> >> > + struct local_datapath *ld) > >> >> >> > +{ > >> >> >> > + const char *peer_name = smap_get(&pb->options, "peer"); > >> >> >> > + if (strcmp(pb->type, "patch") || !peer_name) { > >> >> >> > + return; > >> >> >> > + } > >> >> >> > + > >> >> >> > + const struct sbrec_port_binding *peer; > >> >> >> > + peer = > >> lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > >> >> >> > + peer_name); > >> >> >> > + > >> >> >> > + if (!peer || !peer->datapath) { > >> >> >> > + return; > >> >> >> > + } > >> >> >> > + > >> >> >> > + bool present = false; > >> >> >> > + for (size_t i = 0; i < ld->n_peer_ports; i++) { > >> >> >> > + if (ld->peer_ports[i].local == pb) { > >> >> >> > + present = true; > >> >> >> > break; > >> >> >> > } > >> >> >> > + } > >> >> >> > > >> >> >> > - if (!strcmp(binding_rec->type, "remote")) { > >> >> >> > - continue; > >> >> >> > + if (!present) { > >> >> >> > + ld->n_peer_ports++; > >> >> >> > + if (ld->n_peer_ports > ld->n_allocated_peer_ports) { > >> >> >> > + ld->peer_ports = > >> >> >> > + x2nrealloc(ld->peer_ports, > >> >> >> > + &ld->n_allocated_peer_ports, > >> >> >> > + sizeof *ld->peer_ports); > >> >> >> > + } > >> >> >> > + ld->peer_ports[ld->n_peer_ports - 1].local = pb; > >> >> >> > + ld->peer_ports[ld->n_peer_ports - 1].remote = peer; > >> >> >> > + } > >> >> >> > + > >> >> >> > + struct local_datapath *peer_ld = > >> >> >> > + get_local_datapath(b_ctx_out->local_datapaths, > >> >> >> > + peer->datapath->tunnel_key); > >> >> >> > + if (!peer_ld) { > >> >> >> > + > >> add_local_datapath__(b_ctx_in->sbrec_datapath_binding_by_key, > >> >> >> > + > >> b_ctx_in->sbrec_port_binding_by_datapath, > >> >> >> > + > b_ctx_in->sbrec_port_binding_by_name, > >> >> >> > + peer->datapath, false, > >> >> >> > + 1, b_ctx_out->local_datapaths); > >> >> >> > + return; > >> >> >> > + } > >> >> >> > + > >> >> >> > + for (size_t i = 0; i < peer_ld->n_peer_ports; i++) { > >> >> >> > + if (peer_ld->peer_ports[i].local == peer) { > >> >> >> > + return; > >> >> >> > } > >> >> >> > + } > >> >> >> > > >> >> >> > - if (strcmp(binding_rec->type, "")) { > >> >> >> > - changed = true; > >> >> >> > + peer_ld->n_peer_ports++; > >> >> >> > + if (peer_ld->n_peer_ports > > peer_ld->n_allocated_peer_ports) { > >> >> >> > + peer_ld->peer_ports = > >> >> >> > + x2nrealloc(peer_ld->peer_ports, > >> >> >> > + &peer_ld->n_allocated_peer_ports, > >> >> >> > + sizeof *peer_ld->peer_ports); > >> >> >> > + } > >> >> >> > + peer_ld->peer_ports[peer_ld->n_peer_ports - 1].local = > peer; > >> >> >> > + peer_ld->peer_ports[peer_ld->n_peer_ports - 1].remote = pb; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static void > >> >> >> > +remove_local_datapath_peer_port(const struct sbrec_port_binding > >> *pb, > >> >> >> > + struct local_datapath *ld, > >> >> >> > + struct hmap *local_datapaths) > >> >> >> > +{ > >> >> >> > + size_t i = 0; > >> >> >> > + for (i = 0; i < ld->n_peer_ports; i++) { > >> >> >> > + if (ld->peer_ports[i].local == pb) { > >> >> >> > break; > >> >> >> > } > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (i == ld->n_peer_ports) { > >> >> >> > + return; > >> >> >> > + } > >> >> >> > + > >> >> >> > + const struct sbrec_port_binding *peer = > >> ld->peer_ports[i].remote; > >> >> >> > > >> >> >> > - struct local_binding *lbinding = NULL; > >> >> >> > - if (!binding_rec->parent_port || > >> >> !binding_rec->parent_port[0]) { > >> >> >> > - lbinding = > >> local_binding_find(b_ctx_out->local_bindings, > >> >> >> > - > >> binding_rec->logical_port); > >> >> >> > + /* Possible improvement: We can shrint the allocated peer > ports > >> >> >> > + * if (ld->n_peer_ports < ld->n_allocated_peer_ports / 2). > >> >> >> > + */ > >> >> >> > + ld->peer_ports[i].local = ld->peer_ports[ld->n_peer_ports - > >> >> 1].local; > >> >> >> > + ld->peer_ports[i].remote = ld->peer_ports[ld->n_peer_ports > - > >> >> >> 1].remote; > >> >> >> > + ld->n_peer_ports--; > >> >> >> > + > >> >> >> > + struct local_datapath *peer_ld = > >> >> >> > + get_local_datapath(local_datapaths, > >> >> peer->datapath->tunnel_key); > >> >> >> > + if (peer_ld) { > >> >> >> > + remove_local_datapath_peer_port(peer, peer_ld, > >> >> local_datapaths); > >> >> >> > >> >> >> Sorry that I don't understand this part. peer's peer is the "pb" > that > >> is > >> >> >> passed in the parameter of the current function call, so this > >> recursive > >> >> >> call would only try to remove the "pb", which is already removed > >> (before > >> >> >> calling this function). So I think it is useless. > >> >> > > >> >> > > >> >> > Let's say there is sw0-lr0 of sw0 connected to the peer lr0-sw0 of > lr0. > >> >> > > >> >> > When this function is called for sw0-lr0, it removes the peer ports > >> >> sw0-lr0 from > >> >> > the 'sw0' ld and then it calls recursively for lr0. > >> >> > > >> >> > Yes you're right. It will again call for sw0-lr0 of sw0. But that > would > >> >> be a no-op as > >> >> > the for loop in the beginning cannot find sw0-lr0 in 'sw0' ld.. > Even > >> >> though it > >> >> > is useless are there any issues with it ? Since its recursion and > it > >> has > >> >> proper return > >> >> > conditions, I think it should be fine. > >> >> > > >> >> > Another option is duplicating the code for removing the peer. > >> >> > > >> >> > >> >> OK, I thought it is useless, but it seems needed. > >> >> > >> >> > > >> >> >> > >> >> >> > + } > >> >> >> > +} > >> >> >> > + > >> >> >> > +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; > >> >> >> > + } > >> >> >> > + } > >> >> >> > >> >> >> I think we need to remove ld from the local_datapaths if the > current > >> pb > >> >> is > >> >> >> the only reason for that ld to be exist on this chassis. Also, all > the > >> >> >> related lds connected by patch ports should be removed as well, if > >> there > >> >> >> are no other pbs causing them to be added to this chassis. This is > the > >> >> >> reverse of add_local_datapath__(). It doesn't seem to be easy > without > >> >> going > >> >> >> through all local_lbindings. Maybe this can be done in > >> >> >> binding_handle_ovs_interface_changes() and > >> >> >> binding_handle_port_binding_changes() out of the loop. > >> >> >> > >> >> > > >> >> > I left this unaddressed intentionally as I think it will complicate > the > >> >> code. > >> >> > > >> >> > Patch 3 of this series handles I-P for datapath and when a datapath > is > >> >> deleted, > >> >> > it removes the datapath from local_datapaths if it is present. > >> >> > I think this should be Ok as we will eventually free it. If we > really > >> >> want to address this, > >> >> > I'm fine with it but I'd prefer a follow up patch to do this. > >> >> > > >> >> OK, maybe a comment can be added to make it clear for future > improvement. > >> >> This changes the behavior of current implementation. Now unrelated > >> >> datapaths are removed when certain ports are unbound, and the flows > in > >> the > >> >> HV can be largely reduced, but with this change it may not reduce any > >> more > >> >> until a recompute is triggered. It is not a big problem I think. > >> > > >> > > >> > Sure. I'll add a comment about it. I was also thinking of adding the > same > >> in TODO.rst. > >> > But it looks like TODO.rst needs some cleaning first. > >> > > >> >> > >> >> > > >> >> >> > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +handle_iface_add(const struct ovsrec_interface *iface_rec, > >> >> >> > + const char *iface_id, > >> >> >> > + struct binding_ctx_in *b_ctx_in, > >> >> >> > + struct binding_ctx_out *b_ctx_out, > >> >> >> > + struct hmap *qos_map) > >> >> >> > +{ > >> >> >> > + sset_add(b_ctx_out->local_lports, iface_id); > >> >> >> > + smap_replace(b_ctx_out->local_iface_ids, iface_rec->name, > >> >> iface_id); > >> >> >> > + > >> >> >> > + struct local_binding *lbinding = > >> >> >> > + local_binding_find(b_ctx_out->local_bindings, > iface_id); > >> >> >> > + > >> >> >> > + if (!lbinding) { > >> >> >> > + lbinding = local_binding_create(iface_id, iface_rec, > NULL, > >> >> >> BT_VIF); > >> >> >> > + local_binding_add(b_ctx_out->local_bindings, lbinding); > >> >> >> > + } else { > >> >> >> > + lbinding->iface = iface_rec; > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (!lbinding->pb || strcmp(lbinding->name, > >> >> >> lbinding->pb->logical_port)) { > >> >> >> > + lbinding->pb = lport_lookup_by_name( > >> >> >> > + b_ctx_in->sbrec_port_binding_by_name, > lbinding->name); > >> >> >> > + if (lbinding->pb && !strcmp(lbinding->pb->type, > >> "virtual")) { > >> >> >> > >> >> >> Since we never bind "virtual" ports, this should be a > >> misconfiguration, > >> >> and > >> >> >> it is better to log a warning message to be more clear. > >> >> >> > >> >> >> > + lbinding->pb = NULL; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (lbinding->pb) { > >> >> >> > + if (!consider_vif_lport(lbinding->pb, b_ctx_in, > b_ctx_out, > >> >> >> > + lbinding, qos_map)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + /* Update the child local_binding's iface (if any children) > and > >> >> try > >> >> >> to > >> >> >> > + * claim the container lbindings. */ > >> >> >> > + struct shash_node *node; > >> >> >> > + SHASH_FOR_EACH (node, &lbinding->children) { > >> >> >> > + struct local_binding *child = node->data; > >> >> >> > + child->iface = iface_rec; > >> >> >> > + if (child->type == BT_CONTAINER) { > >> >> >> > + if (!consider_container_lport(child->pb, b_ctx_in, > >> >> b_ctx_out, > >> >> >> > + qos_map)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +handle_iface_delete(const char *lport_name, const char > *iface_name, > >> >> >> > + struct binding_ctx_in *b_ctx_in, > >> >> >> > + struct binding_ctx_out *b_ctx_out, bool > >> *changed) > >> >> >> > +{ > >> >> >> > + struct local_binding *lbinding; > >> >> >> > + lbinding = local_binding_find(b_ctx_out->local_bindings, > >> >> >> > + lport_name); > >> >> >> > + if (lbinding && lbinding->pb && > >> >> >> > + lbinding->pb->chassis == b_ctx_in->chassis_rec) { > >> >> >> > + > >> >> >> > + if (!release_local_binding(b_ctx_in->chassis_rec, > lbinding, > >> >> >> > + !b_ctx_in->ovnsb_idl_txn)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + struct local_datapath *ld = > >> >> >> > + get_local_datapath(b_ctx_out->local_datapaths, > >> >> >> > + > >> lbinding->pb->datapath->tunnel_key); > >> >> >> > + if (ld) { > >> >> >> > + remove_pb_from_local_datapath(lbinding->pb, > >> >> >> > + > b_ctx_in->chassis_rec, > >> >> >> > + b_ctx_out, ld); > >> >> >> > + } > >> >> >> > + local_binding_delete(b_ctx_out->local_bindings, > lbinding); > >> >> >> > + *changed = true; > >> >> >> > + } > >> >> >> > + > >> >> >> > + sset_find_and_delete(b_ctx_out->local_lports, lport_name); > >> >> >> > + smap_remove(b_ctx_out->local_iface_ids, iface_name); > >> >> >> > + > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +is_iface_vif(const struct ovsrec_interface *iface_rec) > >> >> >> > +{ > >> >> >> > + if (iface_rec->type && iface_rec->type[0] && > >> >> >> > + strcmp(iface_rec->type, "internal")) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +/* Returns true if the ovs interface changes were handled > >> >> successfully, > >> >> >> > + * false otherwise. > >> >> >> > + */ > >> >> >> > +bool > >> >> >> > +binding_handle_ovs_interface_changes(struct binding_ctx_in > >> *b_ctx_in, > >> >> >> > + struct binding_ctx_out > >> >> *b_ctx_out, > >> >> >> > + bool *changed) > >> >> >> > +{ > >> >> >> > + if (!b_ctx_in->chassis_rec) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + bool handled = true; > >> >> >> > + *changed = false; > >> >> >> > + > >> >> >> > + /* Run the tracked interfaces loop twice. One to handle > deleted > >> >> >> > + * changes. And another to handle add/update changes. > >> >> >> > + * This will ensure correctness. > >> >> >> > + */ > >> >> >> > + const struct ovsrec_interface *iface_rec; > >> >> >> > + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, > >> >> >> > + > >> b_ctx_in->iface_table) { > >> >> >> > + if (!is_iface_vif(iface_rec)) { > >> >> >> > + /* Right now are not handling ovs_interface changes > of > >> >> >> > + * other types. This can be enhanced to handle of > >> >> >> > + * types - patch and tunnel. */ > >> >> >> > + handled = false; > >> >> >> > + break; > >> >> >> > + } > >> >> >> > + > >> >> >> > + const char *iface_id = > smap_get(&iface_rec->external_ids, > >> >> >> "iface-id"); > >> >> >> > + const char *old_iface_id = > >> >> smap_get(b_ctx_out->local_iface_ids, > >> >> >> > + iface_rec->name); > >> >> >> > + const char *cleared_iface_id = NULL; > >> >> >> > + if (!ovsrec_interface_is_deleted(iface_rec)) { > >> >> >> > + 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 { > >> >> >> > - lbinding = > >> local_binding_find(b_ctx_out->local_bindings, > >> >> >> > - > >> binding_rec->parent_port); > >> >> >> > + cleared_iface_id = iface_id; > >> >> >> > + iface_id = NULL; > >> >> >> > } > >> >> >> > > >> >> >> > - if (lbinding) { > >> >> >> > - changed = true; > >> >> >> > + if (cleared_iface_id) { > >> >> >> > + handled = handle_iface_delete(cleared_iface_id, > >> >> >> iface_rec->name, > >> >> >> > + b_ctx_in, b_ctx_out, > >> >> changed); > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (!handled) { > >> >> >> > break; > >> >> >> > } > >> >> >> > } > >> >> >> > > >> >> >> > - return changed; > >> >> >> > + if (!handled) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > >> >> >> > + struct hmap *qos_map_ptr = > >> >> >> > + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : > &qos_map; > >> >> >> > + > >> >> >> > + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, > >> >> >> > + > >> b_ctx_in->iface_table) { > >> >> >> > + /* Loop to handle create and update changes only. */ > >> >> >> > >> >> >> Thanks for splitting the loops, but here are still some problems. > >> >> >> > >> >> >> 1. The first loop handled: a) interface deletion. b) interface > update > >> >> with > >> >> >> old iface-id cleared. So in this second loop it should handle all > the > >> >> rest > >> >> >> cases: a) interface add. b) interface update without iface-id > change. > >> It > >> >> >> seems b) is treated as just interface add, which may be wrong: if > >> >> iface-id > >> >> >> didn't change but some other fields of the interface record is > >> changed, > >> >> it > >> >> >> is not handled properly. > >> >> > > >> >> > > >> >> > If other fields change, what should we handle ? Can ofport change ? > >> >> > I'll see if ofport can be changed and what happens when it changes. > >> >> > > >> >> > For other fields like name, mtu etc do we need to handle anything ? > >> >> > I think we only care about the external_ids:iface-id and the > ofport. > >> >> > The function handle_iface_add() even if it is called for other > changes, > >> >> > it will be a no-op. > >> >> > > >> >> > Also from the tracking we can't figure out what fields change right > ? > >> >> > > >> >> > > >> >> Yes, I think it is hard to check with fields are changed and handle > it > >> >> accordingly. That's why I suggest to make it simple: if there is an > >> update, > >> >> always handle it like a deletion and then creation. This way, we dont > >> need > >> >> to consider all the corner cases and we are sure it is correct. > >> > > >> > > >> > In this particular case of binding, I don't think there are any corner > >> cases.. > >> > > >> > We consider an OVS interface for binding if the below 2 conditions > meet > >> > > >> > 1. external_ids:iface-id is set > >> > 2. ofport is > 0. > >> > > >> > Otherwise we need to consider a release if it was already claimed. > >> > > >> > When an ovs interface is updated like below > >> > $ovs-vsctl set interface p1 external_ids:foo=bar > >> > > >> > We don't need to handle it like a deletion and then a creation. > Because > >> we can > >> > easily see that the above conditions are met, > >> > > >> What you described is correct (I believe). However, it is the > declarative > >> way of thinking, which is perfect for the always recompute > implementation, > >> but it is hard to reason the correctness and to maintain in longer term > for > >> incremental processing. Incremental processing focuses on the data > change > >> instead of the desired status. In this case you may have already thought > >> through and sure about the correctness even some data change is ignored. > >> However, in the long run, it is hard to remember all the details about > >> which field change needs to be handled in which way, and why some other > >> changes are ignored. More importantly, when a developer who is not so > >> familiar with this changle handling logic want to add a new feature, > which > >> requires another field, e.g. another key-value in external_ids, it is > easy > >> to forget to update here, and not easy to catch in code review either, > and > >> even we do aware it needs to be updated here, the if condition would get > so > >> complex even with just 3 fields to be considered. > > > > > > > > I think adding comments should be enough for a new user. And I'll make > sure > > I add proper comments. > > In the case of binding an interface, I really don't see why would > anything change in the future. Even > > if it changes, I think the test cases should fail if that is the case. > > > > > >> > >> This is why I think we > >> should make it "simple and stupid" to handle updates as if the old one > was > >> deleted and then a new one is added. > > > > > > > > If suppose an OVS interface external_ids:iface-id is updated from 'foo' > to 'bar' > > and if 'foo' logical port was already bound, then we still need to check > what was > > the old value from the 'b_ctx_out->local_iface_ids' and release that port > binding > > before considering claiming the port binding for 'bar'. > > > > In my opinion calling delete followed by create will not help. And we > still need to maintain the > > logic of checking the necessary fields as the ovsdb tracking doesn't say > what changed. > > And I don't think calling delete, followed by create is going to make the > code simpler. Or will work > > without any changes later if there is a need to handle port binding > differently. > > > > Maybe deletion/addition is not accurate here. I didn't mean we don't need > to compare the iface-id. iface-id is a key here and we need to be sure if > it is changed. Since ovsdb tracking doesn't provide the old data, I think > your approach of maintaining the local_binding data structure served that > purpose pretty well. The problem we are discussing here is about how to > handle the changes when we compared and identified the iface-id > change/no-change. So I didn't mean we can simply handle as if a interface > is deleted/added (sorry about my wording). Maybe unbinding/binding is a > better word (or releasing/claiming), which means deletion/addition of a > local_binding entry (and all the data related to it). Now the first loop is > already doing the "releasing", for the second loop, since the rest of cases > include both "claiming" and "updating claiming". My proposal is just to > handle the later in a simple way with confidence. > > The miss in the below condition is just an example why handling each of the > other fields could be complex and error prone: > > >> >> >> > + if (iface_id && ofport > 0) { > >> >> >> > >> >> >> What if iface-id didn't change, but ofport changed from non-zero > to 0? > >> >> How > >> >> >> should we handle the change? > > For the case of "iface-id didn't change, but ofport changed from non-zero > to 0", according to the criteria you gave, the binding should be released. > My proposal is that we don't need to check the ofport here at all. In this > loop, just follow the principle of "releasing the old" and "claiming the > new", we will first release the binding, and then handle the claiming, > which would fail because the criteria for binding is not met.
> Although you could fix it by adding some more condition checks here for > ofport to achieve the same result, I think it makes the change-handler more > complex unnecessarily. If there are more conditions to check in the future > it would be harder to maintain. The conditions check is better to be > encapsulated in the functions that handles the claim/release. > Thanks for the comments. In v8, I have added more comments to make it more cleared. Please take a look and we can discuss further there. Thanks Numan > Similar handlings in lflow processing is, lflow uuid is the key. If a lflow > is changed, delete everything related to the lflow, and re-consider it. > Same for port-binding handling in physical flows. Here, the key is > <iface-id, ifname>. If one of the pair is gone, the related bindings should > be released; if a new pair is added; the related binding should be created; > if an existing pair is updated with whatever fields, release the binding > and reclaim it so than any property of the binding is up to date. To figure > out the addition/deletion/update of the <iface-id, ifname> pair, the > approach you did with local_binding structure is very helpful, which > overcame the ovsdb tracking defect. > > > > >> > >> At least this has provided me a lot of > >> convenience in the earlier I-P implementations, and I was even surprised > >> that there were so many new features added but most of them didn't > needed > >> to be aware of the I-P details, thanks to this principle. I hope this > >> clarifies the idea behind. > > > > > > Ok. But I'm not sure if we should apply this principle as a strict rule. > > > Of course, there will always be exceptions. When applying this rule create > more trouble than benefit we don't need to follow, but at least it is a way > help us to evaluate if there is anything missing in the I-P, to avoid > pitfalls. > > >> > >> > It also creates unnecessary deletion and re-adding of OVS flows > related > >> to that > >> > logical port when we handle flow computation for changed runtime data. > >> > > >> > If we handle this way, there won't be any runtime data changes. > >> > > >> > >> I agree it may be less optimized, but considering that we are > incrementally > >> processing the changes here, even if there are many interfaces deteled > and > >> added unnecesarily, the cost of I-P for such changes is just nothing > >> comparing with the full recompute. I believe it would be good enough > from > >> the end user point of view, if we can make it incrementally processed, > >> correctly. > >> > > > > But the idea of this whole patch series is also to improve the > performance as much > > as we can, because of the scale issues we have seen. > > > I think all the performance problem was because of recomputing, so I guess > the purpose of this series is to do I-P as much as we can. I assume once > the change is handled incrementally, there is no performance problem to be > worried about. For the interface changes, one interface may correspond to > one physical flow only. Even tens of interfaces are changed together on the > chassis we only need to handle tens of flow computations, which is nothing > compared to the millions of flow computations that caused the performance > problems. > > My suggestions here are the principles that I think is helpful to achieve > the correctness and maintenability, but if you think the same has been > thought through and can be achieved in different ways, I respect your > decision ;) > > > I would also like to hear the opinion of Mark, Dumitru and others . > > > > Thanks > > Numan > > > >> > >> > Thanks > >> > Numan > >> > > >> >> > >> >> > > >> >> >> > >> >> >> > >> >> >> 2. Some other condition checks should be done for this loop, such > as: > >> if > >> >> >> (!is_iface_vif(iface_rec)) ... > >> >> > > >> >> > > >> >> > This is not required because the first loop checks for it and if > there > >> is > >> >> any > >> >> > non vif interface change, then this function returns false, before > the > >> >> > second loop. > >> >> > > >> >> OK. > >> >> > >> >> >> > >> >> >> > >> >> >> > + if (ovsrec_interface_is_deleted(iface_rec)) { > >> >> >> > + continue; > >> >> >> > + } > >> >> >> > + > >> >> >> > + const char *iface_id = > smap_get(&iface_rec->external_ids, > >> >> >> "iface-id"); > >> >> >> > + int64_t ofport = iface_rec->n_ofport ? > *iface_rec->ofport > >> : 0; > >> >> >> > + if (iface_id && ofport > 0) { > >> >> >> > >> >> >> What if iface-id didn't change, but ofport changed from non-zero > to 0? > >> >> How > >> >> >> should we handle the change? > >> >> >> > >> >> > > >> >> > Good point. I think we should consider this as > handle_iface_delete. > >> >> > > >> >> > I'll rename the function handle_iface_delete() to > >> handle_iface_release() > >> >> > and handle_iface_add() to handle_iface_claim() to be more clear. > >> >> > > >> >> >> A general approach can be: whenever there is an update > >> >> >> (!ovsrec_interface_is_new()), we firstly handle it as interface > >> delete, > >> >> and > >> >> >> then handle an interface add. > >> >> > > >> >> > > >> >> > But why delete and add if can determine that it is going to be > either > >> one > >> >> > i.e release or claim and I think we can definitely determine. > >> >> > Also in patch 6/7 where we do tracking, it becomes a bit > complicated > >> >> > if we handle it as delete and then add as we may add 2 such > tracking > >> >> > data. > >> >> > > >> >> It is for simplicity and correctness. Please see the comment above. > >> >> > >> >> > Thanks > >> >> > Numan > >> >> > > >> >> >> > >> >> >> > + *changed = true; > >> >> >> > + handled = handle_iface_add(iface_rec, iface_id, > >> b_ctx_in, > >> >> >> > + b_ctx_out, qos_map_ptr); > >> >> >> > + if (!handled) { > >> >> >> > + break; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (handled && qos_map_ptr && > >> set_noop_qos(b_ctx_in->ovs_idl_txn, > >> >> >> > + > >> b_ctx_in->port_table, > >> >> >> > + > b_ctx_in->qos_table, > >> >> >> > + > >> >> >> b_ctx_out->egress_ifaces)) { > >> >> >> > + const char *entry; > >> >> >> > + SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > >> >> >> > + setup_qos(entry, &qos_map); > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + destroy_qos_map(&qos_map); > >> >> >> > + return handled; > >> >> >> > } > >> >> >> > > >> >> >> > -/* Returns true if the database is all cleaned up, false if > more > >> work > >> >> is > >> >> >> > - * required. */ > >> >> >> > -bool > >> >> >> > -binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > >> >> >> > - const struct sbrec_port_binding_table > >> >> >> *port_binding_table, > >> >> >> > - const struct sbrec_chassis *chassis_rec) > >> >> >> > +static void > >> >> >> > +handle_deleted_lport(const struct sbrec_port_binding *pb, > >> >> >> > + struct binding_ctx_in *b_ctx_in, > >> >> >> > + struct binding_ctx_out *b_ctx_out) > >> >> >> > { > >> >> >> > - if (!ovnsb_idl_txn) { > >> >> >> > + struct local_datapath *ld = > >> >> >> > + get_local_datapath(b_ctx_out->local_datapaths, > >> >> >> > + pb->datapath->tunnel_key); > >> >> >> > + if (ld) { > >> >> >> > + remove_pb_from_local_datapath(pb, > b_ctx_in->chassis_rec, > >> >> >> > + b_ctx_out, ld); > >> >> >> > + } > >> >> >> > +} > >> >> >> > + > >> >> >> > +static struct local_binding * > >> >> >> > +get_lbinding_for_lport(const struct sbrec_port_binding *pb, > >> >> >> > + enum en_lport_type lport_type, > >> >> >> > + struct binding_ctx_out *b_ctx_out) > >> >> >> > +{ > >> >> >> > + ovs_assert(lport_type == LP_VIF || lport_type == > LP_VIRTUAL); > >> >> >> > + > >> >> >> > + if (lport_type == LP_VIF && !is_lport_container(pb)) { > >> >> >> > + return local_binding_find(b_ctx_out->local_bindings, > >> >> >> pb->logical_port); > >> >> >> > + } > >> >> >> > + > >> >> >> > + struct local_binding *parent_lbinding = NULL; > >> >> >> > + > >> >> >> > + if (lport_type == LP_VIRTUAL) { > >> >> >> > + parent_lbinding = > >> >> local_binding_find(b_ctx_out->local_bindings, > >> >> >> > + > pb->virtual_parent); > >> >> >> > + } else { > >> >> >> > + parent_lbinding = > >> >> local_binding_find(b_ctx_out->local_bindings, > >> >> >> > + pb->parent_port); > >> >> >> > + } > >> >> >> > + > >> >> >> > + return parent_lbinding > >> >> >> > + ? local_binding_find(&parent_lbinding->children, > >> >> >> pb->logical_port) > >> >> >> > + : NULL; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > >> >> >> > + enum en_lport_type lport_type, > >> >> >> > + struct binding_ctx_in *b_ctx_in, > >> >> >> > + struct binding_ctx_out *b_ctx_out, > >> >> >> > + bool *changed) > >> >> >> > +{ > >> >> >> > + struct local_binding *lbinding = > >> >> >> > + get_lbinding_for_lport(pb, lport_type, b_ctx_out); > >> >> >> > + > >> >> >> > + if (lbinding) { > >> >> >> > + lbinding->pb = NULL; > >> >> >> > + /* The port_binding 'pb' is deleted. So there is no > need to > >> >> >> > + * clear the 'chassis' column of 'pb'. But we need to > do > >> >> >> > + * for the local_binding's children. */ > >> >> >> > + if (lbinding->type == BT_VIF && > >> >> >> > + > >> !release_local_binding_children(b_ctx_in->chassis_rec, > >> >> >> > + lbinding, > >> >> >> > + > >> >> >> !b_ctx_in->ovnsb_idl_txn)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + *changed = true; > >> >> >> > + } > >> >> >> > + > >> >> >> > + handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +handle_updated_vif_lport(const struct sbrec_port_binding *pb, > >> >> >> > + enum en_lport_type lport_type, > >> >> >> > + struct binding_ctx_in *b_ctx_in, > >> >> >> > + struct binding_ctx_out *b_ctx_out, > >> >> >> > + struct hmap *qos_map, > >> >> >> > + bool *changed) > >> >> >> > +{ > >> >> >> > + bool claimed = (pb->chassis == b_ctx_in->chassis_rec); > >> >> >> > + bool handled = true; > >> >> >> > + > >> >> >> > + if (lport_type == LP_VIRTUAL) { > >> >> >> > + handled = consider_virtual_lport(pb, b_ctx_in, > b_ctx_out, > >> >> >> qos_map); > >> >> >> > + } else if (lport_type == LP_VIF && is_lport_container(pb)) > { > >> >> >> > + handled = consider_container_lport(pb, b_ctx_in, > b_ctx_out, > >> >> >> qos_map); > >> >> >> > + } else { > >> >> >> > + handled = consider_vif_lport(pb, b_ctx_in, b_ctx_out, > NULL, > >> >> >> qos_map); > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (!handled) { > >> >> >> > return false; > >> >> >> > } > >> >> >> > - if (!chassis_rec) { > >> >> >> > + > >> >> >> > + bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > >> >> >> > + bool changed_ = (claimed != now_claimed); > >> >> >> > + > >> >> >> > + if (changed_) { > >> >> >> > + *changed = true; > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (lport_type == LP_VIRTUAL || > >> >> >> > + (lport_type == LP_VIF && is_lport_container(pb)) || > >> >> !changed_) { > >> >> >> > return true; > >> >> >> > } > >> >> >> > > >> >> >> > - const struct sbrec_port_binding *binding_rec; > >> >> >> > - bool any_changes = false; > >> >> >> > - SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, > >> >> port_binding_table) { > >> >> >> > - if (binding_rec->chassis == chassis_rec) { > >> >> >> > - if (binding_rec->encap) > >> >> >> > - sbrec_port_binding_set_encap(binding_rec, > NULL); > >> >> >> > - sbrec_port_binding_set_chassis(binding_rec, NULL); > >> >> >> > - any_changes = true; > >> >> >> > + struct local_binding *lbinding = > >> >> >> > + local_binding_find(b_ctx_out->local_bindings, > >> >> pb->logical_port); > >> >> >> > + > >> >> >> > + ovs_assert(lbinding); > >> >> >> > + > >> >> >> > + struct shash_node *node; > >> >> >> > + SHASH_FOR_EACH (node, &lbinding->children) { > >> >> >> > + struct local_binding *child = node->data; > >> >> >> > + if (child->type == BT_CONTAINER) { > >> >> >> > + handled = consider_container_lport(child->pb, > b_ctx_in, > >> >> >> b_ctx_out, > >> >> >> > + qos_map); > >> >> >> > + if (!handled) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > } > >> >> >> > } > >> >> >> > > >> >> >> > - if (any_changes) { > >> >> >> > - ovsdb_idl_txn_add_comment( > >> >> >> > - ovnsb_idl_txn, > >> >> >> > - "ovn-controller: removing all port bindings for > '%s'", > >> >> >> > - chassis_rec->name); > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +/* Returns true if the port binding changes resulted in local > >> binding > >> >> >> > + * updates, false otherwise. > >> >> >> > + */ > >> >> >> > +bool > >> >> >> > +binding_handle_port_binding_changes(struct binding_ctx_in > >> *b_ctx_in, > >> >> >> > + struct binding_ctx_out > >> *b_ctx_out, > >> >> >> > + bool *changed) > >> >> >> > +{ > >> >> >> > + bool handled = true; > >> >> >> > + *changed = false; > >> >> >> > + > >> >> >> > + const struct sbrec_port_binding *pb; > >> >> >> > + > >> >> >> > + /* Run the tracked port binding loop twice. One to handle > >> deleted > >> >> >> > + * changes. And another to handle add/update changes. > >> >> >> > + * This will ensure correctness. > >> >> >> > + */ > >> >> >> > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > >> >> >> > + > >> >> >> b_ctx_in->port_binding_table) { > >> >> >> > + if (!sbrec_port_binding_is_deleted(pb)) { > >> >> >> > + continue; > >> >> >> > + } > >> >> >> > + > >> >> >> > + enum en_lport_type lport_type = get_lport_type(pb); > >> >> >> > + if (lport_type == LP_VIF || lport_type == LP_VIRTUAL) { > >> >> >> > + handled = handle_deleted_vif_lport(pb, lport_type, > >> >> b_ctx_in, > >> >> >> > + b_ctx_out, > >> changed); > >> >> >> > + } else { > >> >> >> > + handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (!handled) { > >> >> >> > + break; > >> >> >> > + } > >> >> >> > } > >> >> >> > > >> >> >> > - return !any_changes; > >> >> >> > + if (!handled) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); > >> >> >> > + struct hmap *qos_map_ptr = > >> >> >> > + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : > &qos_map; > >> >> >> > + > >> >> >> > + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, > >> >> >> > + > >> >> >> b_ctx_in->port_binding_table) { > >> >> >> > + /* Loop to handle create and update changes only. */ > >> >> >> > + if (sbrec_port_binding_is_deleted(pb)) { > >> >> >> > + continue; > >> >> >> > + } > >> >> >> > + > >> >> >> > + enum en_lport_type lport_type = get_lport_type(pb); > >> >> >> > + > >> >> >> > + struct local_datapath *ld = > >> >> >> > + get_local_datapath(b_ctx_out->local_datapaths, > >> >> >> > + pb->datapath->tunnel_key); > >> >> >> > + > >> >> >> > + switch (lport_type) { > >> >> >> > + case LP_VIF: > >> >> >> > + case LP_VIRTUAL: > >> >> >> > + handled = handle_updated_vif_lport(pb, lport_type, > >> >> b_ctx_in, > >> >> >> > + b_ctx_out, > >> qos_map_ptr, > >> >> >> > + changed); > >> >> >> > + break; > >> >> >> > + > >> >> >> > + case LP_PATCH: > >> >> >> > + case LP_LOCALPORT: > >> >> >> > + case LP_VTEP: > >> >> >> > + *changed = true; > >> >> >> > + update_local_lport_ids(b_ctx_out->local_lport_ids, > pb); > >> >> >> > + if (lport_type == LP_PATCH) { > >> >> >> > + /* Add the peer datapath to the local datapaths > if > >> >> it's > >> >> >> > + * not present yet. > >> >> >> > + */ > >> >> >> > + if (ld) { > >> >> >> > + add_local_datapath_peer_port(pb, b_ctx_in, > >> >> >> > + b_ctx_out, > ld); > >> >> >> > + } > >> >> >> > + } > >> >> >> > + break; > >> >> >> > + > >> >> >> > + case LP_L2GATEWAY: > >> >> >> > + *changed = true; > >> >> >> > + handled = consider_l2gw_lport(pb, b_ctx_in, > b_ctx_out); > >> >> >> > + break; > >> >> >> > + > >> >> >> > + case LP_L3GATEWAY: > >> >> >> > + *changed = true; > >> >> >> > + handled = consider_l3gw_lport(pb, b_ctx_in, > b_ctx_out); > >> >> >> > + break; > >> >> >> > + > >> >> >> > + case LP_CHASSISREDIRECT: > >> >> >> > + *changed = true; > >> >> >> > + handled = consider_cr_lport(pb, b_ctx_in, > b_ctx_out); > >> >> >> > + break; > >> >> >> > + > >> >> >> > + case LP_EXTERNAL: > >> >> >> > + *changed = true; > >> >> >> > + handled = consider_external_lport(pb, b_ctx_in, > >> >> b_ctx_out); > >> >> >> > + break; > >> >> >> > + > >> >> >> > + case LP_LOCALNET: { > >> >> >> > + *changed = true; > >> >> >> > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > >> >> >> qos_map_ptr); > >> >> >> > + > >> >> >> > + struct shash bridge_mappings = > >> >> >> > + SHASH_INITIALIZER(&bridge_mappings); > >> >> >> > + add_ovs_bridge_mappings(b_ctx_in->ovs_table, > >> >> >> > + b_ctx_in->bridge_table, > >> >> >> > + &bridge_mappings); > >> >> >> > + update_ld_localnet_port(pb, &bridge_mappings, > >> >> >> > + b_ctx_out->egress_ifaces, > >> >> >> > + > b_ctx_out->local_datapaths); > >> >> >> > + shash_destroy(&bridge_mappings); > >> >> >> > + break; > >> >> >> > + } > >> >> >> > + > >> >> >> > + case LP_REMOTE: > >> >> >> > + case LP_UNKNOWN: > >> >> >> > + break; > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (!handled) { > >> >> >> > + break; > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (handled && qos_map_ptr && > >> set_noop_qos(b_ctx_in->ovs_idl_txn, > >> >> >> > + > >> b_ctx_in->port_table, > >> >> >> > + > b_ctx_in->qos_table, > >> >> >> > + > >> >> >> b_ctx_out->egress_ifaces)) { > >> >> >> > + const char *entry; > >> >> >> > + SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) { > >> >> >> > + setup_qos(entry, &qos_map); > >> >> >> > + } > >> >> >> > + } > >> >> >> > + > >> >> >> > + destroy_qos_map(&qos_map); > >> >> >> > + return handled; > >> >> >> > } > >> >> >> > diff --git a/controller/binding.h b/controller/binding.h > >> >> >> > index 9affc9a96..f7ace6faf 100644 > >> >> >> > --- a/controller/binding.h > >> >> >> > +++ b/controller/binding.h > >> >> >> > @@ -57,6 +57,7 @@ struct binding_ctx_out { > >> >> >> > struct sset *local_lports; > >> >> >> > struct sset *local_lport_ids; > >> >> >> > struct sset *egress_ifaces; > >> >> >> > + struct smap *local_iface_ids; > >> >> >> > }; > >> >> >> > > >> >> >> > void binding_register_ovs_idl(struct ovsdb_idl *); > >> >> >> > @@ -64,9 +65,13 @@ void binding_run(struct binding_ctx_in *, > struct > >> >> >> binding_ctx_out *); > >> >> >> > bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, > >> >> >> > const struct sbrec_port_binding_table *, > >> >> >> > const struct sbrec_chassis *); > >> >> >> > -bool binding_evaluate_port_binding_changes(struct > binding_ctx_in *, > >> >> >> > - struct > binding_ctx_out > >> *); > >> >> >> > > >> >> >> > void local_bindings_init(struct shash *local_bindings); > >> >> >> > void local_bindings_destroy(struct shash *local_bindings); > >> >> >> > +bool binding_handle_ovs_interface_changes(struct binding_ctx_in > *, > >> >> >> > + struct > binding_ctx_out *, > >> >> >> > + bool *changed); > >> >> >> > +bool binding_handle_port_binding_changes(struct binding_ctx_in > *, > >> >> >> > + struct binding_ctx_out > *, > >> >> >> > + bool *changed); > >> >> >> > #endif /* controller/binding.h */ > >> >> >> > diff --git a/controller/ovn-controller.c > >> b/controller/ovn-controller.c > >> >> >> > index 6b759966b..08b074415 100644 > >> >> >> > --- a/controller/ovn-controller.c > >> >> >> > +++ b/controller/ovn-controller.c > >> >> >> > @@ -753,6 +753,7 @@ enum sb_engine_node { > >> >> >> > OVS_NODE(open_vswitch, "open_vswitch") \ > >> >> >> > OVS_NODE(bridge, "bridge") \ > >> >> >> > OVS_NODE(port, "port") \ > >> >> >> > + OVS_NODE(interface, "interface") \ > >> >> >> > OVS_NODE(qos, "qos") > >> >> >> > > >> >> >> > enum ovs_engine_node { > >> >> >> > @@ -974,6 +975,7 @@ struct ed_type_runtime_data { > >> >> >> > struct sset active_tunnels; > >> >> >> > > >> >> >> > struct sset egress_ifaces; > >> >> >> > + struct smap local_iface_ids; > >> >> >> > }; > >> >> >> > > >> >> >> > static void * > >> >> >> > @@ -987,6 +989,7 @@ en_runtime_data_init(struct engine_node > *node > >> >> >> OVS_UNUSED, > >> >> >> > sset_init(&data->local_lport_ids); > >> >> >> > sset_init(&data->active_tunnels); > >> >> >> > sset_init(&data->egress_ifaces); > >> >> >> > + smap_init(&data->local_iface_ids); > >> >> >> > local_bindings_init(&data->local_bindings); > >> >> >> > return data; > >> >> >> > } > >> >> >> > @@ -1000,6 +1003,7 @@ en_runtime_data_cleanup(void *data) > >> >> >> > sset_destroy(&rt_data->local_lport_ids); > >> >> >> > sset_destroy(&rt_data->active_tunnels); > >> >> >> > sset_destroy(&rt_data->egress_ifaces); > >> >> >> > + smap_destroy(&rt_data->local_iface_ids); > >> >> >> > struct local_datapath *cur_node, *next_node; > >> >> >> > HMAP_FOR_EACH_SAFE (cur_node, next_node, hmap_node, > >> >> >> > &rt_data->local_datapaths) { > >> >> >> > @@ -1041,6 +1045,10 @@ init_binding_ctx(struct engine_node > *node, > >> >> >> > (struct ovsrec_port_table *)EN_OVSDB_GET( > >> >> >> > engine_get_input("OVS_port", node)); > >> >> >> > > >> >> >> > + struct ovsrec_interface_table *iface_table = > >> >> >> > + (struct ovsrec_interface_table *)EN_OVSDB_GET( > >> >> >> > + engine_get_input("OVS_interface", node)); > >> >> >> > + > >> >> >> > struct ovsrec_qos_table *qos_table = > >> >> >> > (struct ovsrec_qos_table *)EN_OVSDB_GET( > >> >> >> > engine_get_input("OVS_qos", node)); > >> >> >> > @@ -1070,6 +1078,7 @@ init_binding_ctx(struct engine_node *node, > >> >> >> > b_ctx_in->sbrec_port_binding_by_datapath = > >> >> >> sbrec_port_binding_by_datapath; > >> >> >> > b_ctx_in->sbrec_port_binding_by_name = > >> sbrec_port_binding_by_name; > >> >> >> > b_ctx_in->port_table = port_table; > >> >> >> > + b_ctx_in->iface_table = iface_table; > >> >> >> > b_ctx_in->qos_table = qos_table; > >> >> >> > b_ctx_in->port_binding_table = pb_table; > >> >> >> > b_ctx_in->br_int = br_int; > >> >> >> > @@ -1083,6 +1092,7 @@ init_binding_ctx(struct engine_node *node, > >> >> >> > b_ctx_out->local_lport_ids = &rt_data->local_lport_ids; > >> >> >> > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > >> >> >> > b_ctx_out->local_bindings = &rt_data->local_bindings; > >> >> >> > + b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > >> >> >> > } > >> >> >> > > >> >> >> > static void > >> >> >> > @@ -1111,10 +1121,12 @@ en_runtime_data_run(struct engine_node > >> *node, > >> >> >> void *data) > >> >> >> > sset_destroy(local_lport_ids); > >> >> >> > sset_destroy(active_tunnels); > >> >> >> > sset_destroy(&rt_data->egress_ifaces); > >> >> >> > + smap_destroy(&rt_data->local_iface_ids); > >> >> >> > sset_init(local_lports); > >> >> >> > sset_init(local_lport_ids); > >> >> >> > sset_init(active_tunnels); > >> >> >> > sset_init(&rt_data->egress_ifaces); > >> >> >> > + smap_init(&rt_data->local_iface_ids); > >> >> >> > local_bindings_init(&rt_data->local_bindings); > >> >> >> > } > >> >> >> > > >> >> >> > @@ -1140,6 +1152,34 @@ en_runtime_data_run(struct engine_node > *node, > >> >> void > >> >> >> *data) > >> >> >> > engine_set_node_state(node, EN_UPDATED); > >> >> >> > } > >> >> >> > > >> >> >> > +static bool > >> >> >> > +runtime_data_ovs_interface_handler(struct engine_node *node, > void > >> >> *data) > >> >> >> > +{ > >> >> >> > + struct ed_type_runtime_data *rt_data = data; > >> >> >> > + struct binding_ctx_in b_ctx_in; > >> >> >> > + struct binding_ctx_out b_ctx_out; > >> >> >> > + init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > >> >> >> > + > >> >> >> > + bool changed = false; > >> >> >> > + if (!binding_handle_ovs_interface_changes(&b_ctx_in, > >> &b_ctx_out, > >> >> >> > + &changed)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + if (changed) { > >> >> >> > + engine_set_node_state(node, EN_UPDATED); > >> >> >> > + } > >> >> >> > + > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > +static bool > >> >> >> > +runtime_data_noop_handler(struct engine_node *node OVS_UNUSED, > >> >> >> > + void *data OVS_UNUSED) > >> >> >> > +{ > >> >> >> > + return true; > >> >> >> > +} > >> >> >> > + > >> >> >> > static bool > >> >> >> > runtime_data_sb_port_binding_handler(struct engine_node *node, > void > >> >> >> *data) > >> >> >> > { > >> >> >> > @@ -1147,11 +1187,21 @@ > runtime_data_sb_port_binding_handler(struct > >> >> >> engine_node *node, void *data) > >> >> >> > struct binding_ctx_in b_ctx_in; > >> >> >> > struct binding_ctx_out b_ctx_out; > >> >> >> > init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > >> >> >> > + if (!b_ctx_in.chassis_rec) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > + > >> >> >> > + bool changed = false; > >> >> >> > + if (!binding_handle_port_binding_changes(&b_ctx_in, > &b_ctx_out, > >> >> >> > + &changed)) { > >> >> >> > + return false; > >> >> >> > + } > >> >> >> > > >> >> >> > - bool changed = > binding_evaluate_port_binding_changes(&b_ctx_in, > >> >> >> > - > >> &b_ctx_out); > >> >> >> > + if (changed) { > >> >> >> > + engine_set_node_state(node, EN_UPDATED); > >> >> >> > + } > >> >> >> > > >> >> >> > - return !changed; > >> >> >> > + return true; > >> >> >> > } > >> >> >> > > >> >> >> > /* Connection tracking zones. */ > >> >> >> > @@ -1894,7 +1944,10 @@ main(int argc, char *argv[]) > >> >> >> > > >> >> >> > engine_add_input(&en_runtime_data, &en_ovs_open_vswitch, > NULL); > >> >> >> > engine_add_input(&en_runtime_data, &en_ovs_bridge, NULL); > >> >> >> > - engine_add_input(&en_runtime_data, &en_ovs_port, NULL); > >> >> >> > + engine_add_input(&en_runtime_data, &en_ovs_port, > >> >> >> > + runtime_data_noop_handler); > >> >> >> > + engine_add_input(&en_runtime_data, &en_ovs_interface, > >> >> >> > + runtime_data_ovs_interface_handler); > >> >> >> > engine_add_input(&en_runtime_data, &en_ovs_qos, NULL); > >> >> >> > > >> >> >> > engine_add_input(&en_runtime_data, &en_sb_chassis, NULL); > >> >> >> > diff --git a/tests/ovn-performance.at b/tests/ > ovn-performance.at > >> >> >> > index a8a15f8fe..5dfc6f7ca 100644 > >> >> >> > --- a/tests/ovn-performance.at > >> >> >> > +++ b/tests/ovn-performance.at > >> >> >> > @@ -239,6 +239,19 @@ for i in 1 2; do > >> >> >> > ovn_attach n1 br-phys 192.168.0.$i > >> >> >> > done > >> >> >> > > >> >> >> > +# Wait for the tunnel ports to be created and up. > >> >> >> > +# Otherwise this may affect the lflow_run count. > >> >> >> > + > >> >> >> > +OVS_WAIT_UNTIL([ > >> >> >> > + test $(as hv1 ovs-vsctl list interface ovn-hv2-0 | \ > >> >> >> > +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1 > >> >> >> > +]) > >> >> >> > + > >> >> >> > +OVS_WAIT_UNTIL([ > >> >> >> > + test $(as hv2 ovs-vsctl list interface ovn-hv1-0 | \ > >> >> >> > +grep tunnel_egress_iface_carrier=up | wc -l) -eq 1 > >> >> >> > +]) > >> >> >> > + > >> >> >> > # Add router lr1 > >> >> >> > OVN_CONTROLLER_EXPECT_HIT( > >> >> >> > [hv1 hv2], [lflow_run], > >> >> >> > -- > >> >> >> > 2.26.2 > >> >> >> > > >> >> >> > _______________________________________________ > >> >> >> > dev mailing list > >> >> >> > 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 > >> >> >> > >> >> _______________________________________________ > >> >> 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 > >> > _______________________________________________ > 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