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
