[ovs-dev] [PATCH ovn v7 2/9] ovn-controller: I-P for SB port binding and OVS interface in runtime_data.
Numan Siddique
numans at ovn.org
Thu May 21 10:48:45 UTC 2020
On Thu, May 21, 2020 at 6:57 AM Han Zhou <hzhou at 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 <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > This patch handles SB port binding changes and OVS interface changes
> > incrementally in the runtime_data stage which otherwise would have
> > resulted in calls to binding_run().
> >
> > Prior to this patch, port binding changes were handled differently.
> > The changes were only evaluated to see if they need full recompute
> > or they can be ignored.
> >
> > With this patch, the runtime data is updated with the changes (both
> > SB port binding and OVS interface) and ports are claimed/released in
> > the handlers itself, avoiding the need to trigger recompute of runtime
> > data stage.
> >
> > Acked-by: Mark Michelson <mmichels at redhat.com>
> > Signed-off-by: Numan Siddique <numans at 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.
> > + }
> > +}
> > +
> > +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.
> > +}
> > +
> > +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 ?
>
> 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.
>
> > + 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.
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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list