Thanks Mark and Dumitru for the reviews. I've addressed almost all the comments and submitted v5. Request to please take a look.
Please see a few comments inline below. Thanks Numan On Thu, Mar 25, 2021 at 6:52 PM Dumitru Ceara <[email protected]> wrote: > > On 3/23/21 4:55 PM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > When a port binding type changes from type 'A' to type 'B', then > > there are many code paths in the existing binding.c which results > > in crashes due to use-after-free or NULL references. > > > > Below crashes are seen when a container lport is changed to a normal > > lport and then deleted. > > > > *** > > (gdb) bt > > 0 in raise () from /lib64/libc.so.6 > > 1 in abort () from /lib64/libc.so.6 > > 2 ovs_abort_valist ("%s: assertion %s failed in %s()") at lib/util.c:419 > > 3 vlog_abort_valist ("%s: assertion %s failed in %s()") at > > lib/vlog.c:1249 > > 4 vlog_abort ("%s: assertion %s failed in %s()") at lib/vlog.c:1263 > > 5 ovs_assert_failure (where="lib/ovsdb-idl.c:4653", > > function="ovsdb_idl_txn_write__", > > condition="row->new_datum != NULL") at > > lib/util.c:86 > > 6 ovsdb_idl_txn_write__ () at lib/ovsdb-idl.c:4695 > > 7 ovsdb_idl_txn_write_clone () at lib/ovsdb-idl.c:4757 > > 8 sbrec_port_binding_set_chassis () at lib/ovn-sb-idl.c:25946 > > 9 release_lport () at controller/binding.c:971 > > 10 release_local_binding_children () at controller/binding.c:1039 > > 11 release_local_binding () at controller/binding.c:1056 > > 12 consider_iface_release (iface_rec=.. > > iface_id="bb43e818-b2ee-4329-b67e-218556580056") at > > controller/binding.c:1880 > > 13 binding_handle_ovs_interface_changes () at controller/binding.c:1998 > > 14 runtime_data_ovs_interface_handler () at > > controller/ovn-controller.c:1481 > > 15 engine_compute () at lib/inc-proc-eng.c:306 > > 16 engine_run_node () at lib/inc-proc-eng.c:352 > > 17 engine_run () at lib/inc-proc-eng.c:377 > > 18 main () at controller/ovn-controller.c:2826 > > > > The present code creates a 'struct local_binding' instance for a > > container lport and adds this object to the parent local binding > > children list. And if the container lport is changed to a normal > > vif, then there is no way to access the local binding object created > > earlier. This patch fixes these type of issues by refactoring the > > 'local binding' code of binding.c. This patch now creates only one > > instance of 'struct local_binding' for every OVS interface with > > external_ids:iface-id set. A new structure 'struct binding_lport' is > > added which is created for a VIF, container and virtual port bindings > > and is stored in 'binding_lports' shash. 'struct local_binding' now > > maintains a list of binding_lports which it maps to. > > > > When a container lport is changed to a normal lport, we now can > > easily access the 'binding_lport' object of the container lport > > fron the 'binding_lports' shash. > > > > Reported-by: Dumitru Ceara <[email protected]> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1936328 > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1936331 > > > > Co-authored-by: Dumitru Ceara <[email protected]> > > Signed-off-by: Dumitru Ceara <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > Hi Numan, > > > > > v3 -> v4 > > --- > > * -> Addressed some of the review comments from Dumitru. > > Added another test case to test the scenarion when a container > > port changes its parent from one to another port. > > > > v2 -> v3 > > ---- > > * Fixed a crash seen when a container port is changed to normal port > > and then deleted in the same transaction. Added the relevant test case > > for this. > > > > v1 -> v2 > > ---- > > * Fixed a crash seen when 2 container ports are changed to normal ports > > in the same transaction. Added the relevant test case for this. > > > > controller/binding.c | 973 +++++++++++++++++++++++------------- > > controller/binding.h | 32 +- > > controller/ovn-controller.c | 25 +- > > controller/physical.c | 13 +- > > tests/ovn.at | 304 +++++++++++ > > 5 files changed, 949 insertions(+), 398 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 4e6c756969..0e3e4aad91 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -597,6 +597,23 @@ remove_local_lport_ids(const struct sbrec_port_binding > > *pb, > > } > > } > > > > +/* Corresponds to each Port_Binding.type. */ > > +enum en_lport_type { > > + LP_UNKNOWN, > > + LP_VIF, > > + LP_CONTAINER, > > + LP_PATCH, > > + LP_L3GATEWAY, > > + LP_LOCALNET, > > + LP_LOCALPORT, > > + LP_L2GATEWAY, > > + LP_VTEP, > > + LP_CHASSISREDIRECT, > > + LP_VIRTUAL, > > + LP_EXTERNAL, > > + LP_REMOTE > > +}; > > + > > /* Local bindings. binding.c module binds the logical port (represented by > > * Port_Binding rows) and sets the 'chassis' column when it sees the > > * OVS interface row (of type "" or "internal") with the > > @@ -608,134 +625,116 @@ remove_local_lport_ids(const struct > > sbrec_port_binding *pb, > > * 'struct local_binding' is used. A shash of these local bindings is > > * maintained with the 'external_ids:iface-id' as the key to the shash. > > * > > - * struct local_binding (defined in binding.h) has 3 main fields: > > - * - type > > + * struct local_binding has 3 main fields: > > + * - name > > * - OVS interface row object > > - * - Port_Binding row object > > - * > > - * An instance of 'struct local_binding' can be one of 3 types. > > - * > > - * BT_VIF: Represent a local binding for an OVS interface of > > - * type "" or "internal" with the external_ids:iface-id > > - * set. > > - * > > - * This can be a > > - * * probable local binding - external_ids:iface-id is > > - * set, but the corresponding Port_Binding row is not > > - * created or is not visible to the local ovn-controller > > - * instance. > > - * > > - * * a local binding - external_ids:iface-id is set and > > - * which is already bound to the corresponding > > Port_Binding > > - * row. > > - * > > - * It maintains a list of children > > - * (of type BT_CONTAINER/BT_VIRTUAL) if any. > > - * > > - * BT_CONTAINER: Represents a local binding which has a parent of type > > - * BT_VIF. Its Port_Binding row's 'parent' column is set > > to > > - * its parent's Port_Binding. It shares the OVS interface > > row > > - * with the parent. > > - * Each ovn-controller when it sees a container > > Port_Binding, > > - * it creates 'struct local_binding' for the parent > > - * Port_Binding and for its even if the OVS interface row > > for > > - * the parent is not present. > > - * > > - * BT_VIRTUAL: Represents a local binding which has a parent of type > > BT_VIF. > > - * Its Port_Binding type is "virtual" and it shares the OVS > > - * interface row with the parent. > > - * Port_Binding of type "virtual" is claimed by pinctrl module > > - * when it sees the ARP packet from the parent's VIF. > > - * > > + * - List of 'binding_lport' objects with the primary lport > > + * in the front of the list (if present). > > * > > * An object of 'struct local_binding' is created: > > - * - For each interface that has iface-id configured with the type - > > BT_VIF. > > + * - For each interface that has iface-id configured. > > * > > - * - For each container Port Binding (of type BT_CONTAINER) and its > > - * parent Port_Binding (of type BT_VIF), no matter if > > - * they are bound to this chassis i.e even if OVS interface row for > > the > > - * parent is not present. > > - * > > - * - For each 'virtual' Port Binding (of type BT_VIRTUAL) provided its > > parent > > - * is bound to this chassis. > > + * - For each port binding (also referred as lport) of type 'LP_VIF' > > + * if it is a parent lport of container lports even if there is no > > + * corresponding OVS interface. > > */ > > > > Nit: This empty line is not really needed. Done. > > > > -static struct local_binding * > > -local_binding_create(const char *name, const struct ovsrec_interface > > *iface, > > - const struct sbrec_port_binding *pb, > > - enum local_binding_type type) > > -{ > > - struct local_binding *lbinding = xzalloc(sizeof *lbinding); > > - lbinding->name = xstrdup(name); > > - lbinding->type = type; > > - lbinding->pb = pb; > > - lbinding->iface = iface; > > - shash_init(&lbinding->children); > > - return lbinding; > > -} > > +struct local_binding { > > + char *name; > > + const struct ovsrec_interface *iface; > > + struct ovs_list binding_lports; > > +}; > > > > -static void > > -local_binding_add(struct shash *local_bindings, struct local_binding > > *lbinding) > > -{ > > - shash_add(local_bindings, lbinding->name, lbinding); > > -} > > +/* This structure represents a logical port (or port binding) > > + * which is associated with 'struct local_binding'. > > + * > > + * An instance of 'struct binding_lport' is created for a logical port > > + * - If the OVS interface's iface-id corresponds to the logical port. > > + * - If it is a container or virtual logical port and its parent > > + * has a 'local binding'. > > + * > > + */ > > +struct binding_lport { > > + struct ovs_list list_node; /* Node in local_binding.binding_lports. */ > > > > -static void > > -local_binding_destroy(struct local_binding *lbinding) > > -{ > > - local_bindings_destroy(&lbinding->children); > > + char *name; > > + const struct sbrec_port_binding *pb; > > + struct local_binding *lbinding; > > + enum en_lport_type type; > > +}; > > > > - free(lbinding->name); > > - free(lbinding); > > -} > > +static struct local_binding *local_binding_create( > > + const char *name, const struct ovsrec_interface *); > > +static void local_binding_add(struct shash *local_bindings, > > + struct local_binding *); > > +static struct local_binding *local_binding_find( > > + struct shash *local_bindings, const char *name); > > +static void local_binding_destroy(struct local_binding *, > > + struct shash *binding_lports); > > +static void local_binding_delete(struct local_binding *, > > + struct shash *local_bindings, > > + struct shash *binding_lports); > > +static struct binding_lport *local_binding_add_lport( > > + struct shash *binding_lports, > > + struct local_binding *, > > + const struct sbrec_port_binding *, > > + enum en_lport_type); > > +static struct binding_lport *local_binding_get_primary_lport( > > + struct local_binding *); > > +static bool local_binding_handle_stale_binding_lports( > > + struct local_binding *lbinding, struct binding_ctx_in *b_ctx_in, > > + struct binding_ctx_out *b_ctx_out, struct hmap *qos_map); > > + > > +static struct binding_lport *binding_lport_create( > > + const struct sbrec_port_binding *, > > + struct local_binding *, enum en_lport_type); > > +static void binding_lport_destroy(struct binding_lport *); > > +static void binding_lport_delete(struct shash *binding_lports, > > + struct binding_lport *); > > +static void binding_lport_add(struct shash *binding_lports, > > + struct binding_lport *); > > +static struct binding_lport *binding_lport_find( > > + struct shash *binding_lports, const char *lport_name); > > +static const struct sbrec_port_binding *binding_lport_get_parent_pb( > > + struct binding_lport *b_lprt); > > +static struct binding_lport *binding_lport_check_and_cleanup( > > + struct binding_lport *, enum en_lport_type, struct shash *b_lports); > > > > void > > -local_bindings_init(struct shash *local_bindings) > > +local_binding_data_init(struct local_binding_data *lbinding_data) > > { > > - shash_init(local_bindings); > > + shash_init(&lbinding_data->bindings); > > + shash_init(&lbinding_data->lports); > > } > > > > void > > -local_bindings_destroy(struct shash *local_bindings) > > +local_binding_data_destroy(struct local_binding_data *lbinding_data) > > { > > struct shash_node *node, *next; > > - SHASH_FOR_EACH_SAFE (node, next, local_bindings) { > > - struct local_binding *lbinding = node->data; > > - local_binding_destroy(lbinding); > > - shash_delete(local_bindings, node); > > - } > > > > - shash_destroy(local_bindings); > > -} > > + SHASH_FOR_EACH_SAFE (node, next, &lbinding_data->lports) { > > + struct binding_lport *b_lport = node->data; > > + binding_lport_destroy(b_lport); > > + shash_delete(&lbinding_data->lports, node); > > + } > > > > -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); > > -} > > + SHASH_FOR_EACH_SAFE (node, next, &lbinding_data->bindings) { > > + struct local_binding *lbinding = node->data; > > + local_binding_destroy(lbinding, &lbinding_data->lports); > > + shash_delete(&lbinding_data->bindings, node); > > + } > > > > -static void > > -local_binding_add_child(struct local_binding *lbinding, > > - struct local_binding *child) > > -{ > > - local_binding_add(&lbinding->children, child); > > - child->parent = lbinding; > > + shash_destroy(&lbinding_data->lports); > > + shash_destroy(&lbinding_data->bindings); > > } > > > > -static struct local_binding * > > -local_binding_find_child(struct local_binding *lbinding, > > - const char *child_name) > > +const struct sbrec_port_binding * > > +local_binding_get_primary_pb(struct shash *local_bindings, const char > > *name) > > Would it be a bit more clear if we replace 'name' with 'pb_name' (in > binding.h too)? Ack. Done in v5. > > > > { > > - return local_binding_find(&lbinding->children, child_name); > > -} > > + struct local_binding *lbinding = local_binding_find(local_bindings, > > name); > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > > > -static void > > -local_binding_delete_child(struct local_binding *lbinding, > > - struct local_binding *child) > > -{ > > - shash_find_and_delete(&lbinding->children, child->name); > > + return b_lport ? b_lport->pb : NULL; > > } > > > > static bool > > @@ -818,26 +817,13 @@ binding_tracked_dp_destroy(struct hmap > > *tracked_datapaths) > > hmap_destroy(tracked_datapaths); > > } > > > > -/* Corresponds to each Port_Binding.type. */ > > -enum en_lport_type { > > - LP_UNKNOWN, > > - LP_VIF, > > - LP_PATCH, > > - LP_L3GATEWAY, > > - LP_LOCALNET, > > - LP_LOCALPORT, > > - LP_L2GATEWAY, > > - LP_VTEP, > > - LP_CHASSISREDIRECT, > > - LP_VIRTUAL, > > - LP_EXTERNAL, > > - LP_REMOTE > > -}; > > - > > static enum en_lport_type > > get_lport_type(const struct sbrec_port_binding *pb) > > { > > if (is_lport_vif(pb)) { > > + if (is_lport_container(pb)) { > > Nit: If I'm not wrong this is the only place where we use > is_lport_container() and its implementation calls is_lport_vif(pb) > again. Does it make sense to just inline the check on pb->parent_port here? Ack. Done in v5. > > > > + return LP_CONTAINER; > > + } > > return LP_VIF; > > } else if (!strcmp(pb->type, "patch")) { > > return LP_PATCH; > > @@ -991,14 +977,14 @@ release_lport(const struct sbrec_port_binding *pb, > > bool sb_readonly, > > static bool > > is_lbinding_set(struct local_binding *lbinding) > > { > > - return lbinding && lbinding->pb && lbinding->iface; > > + return lbinding && lbinding->iface; > > } > > > > static bool > > -is_lbinding_this_chassis(struct local_binding *lbinding, > > - const struct sbrec_chassis *chassis) > > +is_binding_lport_this_chassis(struct binding_lport *b_lport, > > + const struct sbrec_chassis *chassis) > > { > > - return lbinding && lbinding->pb && lbinding->pb->chassis == chassis; > > + return b_lport && b_lport->pb && b_lport->pb->chassis == chassis; > > If I'm not wrong, there are code paths that end up here with 'chassis' > NULL, (e.g., if sb_readonly). Should we make sure that we don't return > true when both b_lport->pb->chassis and 'chassis' are NULL? Ack. I add a check for this in v5. > > > > } > > > > static bool > > @@ -1010,15 +996,14 @@ can_bind_on_this_chassis(const struct sbrec_chassis > > *chassis_rec, > > || !strcmp(requested_chassis, chassis_rec->hostname); > > } > > > > -/* Returns 'true' if the 'lbinding' has children of type BT_CONTAINER, > > +/* Returns 'true' if the 'lbinding' has binding lports of type > > LP_CONTAINER, > > * 'false' otherwise. */ > > static bool > > is_lbinding_container_parent(struct local_binding *lbinding) > > { > > - struct shash_node *node; > > - SHASH_FOR_EACH (node, &lbinding->children) { > > - struct local_binding *l = node->data; > > - if (l->type == BT_CONTAINER) { > > + struct binding_lport *b_lport; > > + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > + if (b_lport->type == LP_CONTAINER) { > > return true; > > } > > } > > @@ -1027,66 +1012,39 @@ is_lbinding_container_parent(struct local_binding > > *lbinding) > > } > > > > static bool > > -release_local_binding_children(const struct sbrec_chassis *chassis_rec, > > - struct local_binding *lbinding, > > - bool sb_readonly, > > - struct hmap *tracked_dp_bindings) > > +release_binding_lport(const struct sbrec_chassis *chassis_rec, > > + struct binding_lport *b_lport, bool sb_readonly, > > + struct hmap *tracked_dp_bindings) > > { > > - 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, tracked_dp_bindings)) { > > - return false; > > - } > > + if (is_binding_lport_this_chassis(b_lport, chassis_rec)) { > > + if (!release_lport(b_lport->pb, sb_readonly, tracked_dp_bindings)) > > { > > + return false; > > } > > - > > - /* Clear the local bindings' 'iface'. */ > > - l->iface = NULL; > > } > > > > return true; > > } > > > > -static bool > > -release_local_binding(const struct sbrec_chassis *chassis_rec, > > - struct local_binding *lbinding, bool sb_readonly, > > - struct hmap *tracked_dp_bindings) > > -{ > > - if (!release_local_binding_children(chassis_rec, lbinding, > > - sb_readonly, tracked_dp_bindings)) > > { > > - return false; > > - } > > - > > - bool retval = true; > > - if (is_lbinding_this_chassis(lbinding, chassis_rec)) { > > - retval = release_lport(lbinding->pb, sb_readonly, > > tracked_dp_bindings); > > - } > > - > > - lbinding->pb = NULL; > > - lbinding->iface = NULL; > > - return retval; > > -} > > - > > 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, > > struct binding_ctx_out *b_ctx_out, > > - struct local_binding *lbinding, > > + struct binding_lport *b_lport, > > struct hmap *qos_map) > > { > > - bool lbinding_set = is_lbinding_set(lbinding); > > + bool lbinding_set = b_lport ? is_lbinding_set(b_lport->lbinding) : > > false; > > Nit: This can be rewritten as: > > bool lbinding_set = b_lport && is_lbinding_set(b_lport->lbinding); Done. > > > > + > > if (lbinding_set) { > > if (can_bind) { > > /* We can claim the lport. */ > > const struct sbrec_port_binding *parent_pb = > > - lbinding->parent ? lbinding->parent->pb : NULL; > > + binding_lport_get_parent_pb(b_lport); > > > > if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec, > > - lbinding->iface, !b_ctx_in->ovnsb_idl_txn, > > - !lbinding->parent, > > - b_ctx_out->tracked_dp_bindings)){ > > + b_lport->lbinding->iface, > > + !b_ctx_in->ovnsb_idl_txn, > > + !parent_pb, b_ctx_out->tracked_dp_bindings)){ > > return false; > > } > > > > @@ -1098,7 +1056,7 @@ consider_vif_lport_(const struct sbrec_port_binding > > *pb, > > b_ctx_out->tracked_dp_bindings); > > update_local_lport_ids(pb, b_ctx_out); > > update_local_lports(pb->logical_port, b_ctx_out); > > - if (lbinding->iface && qos_map && b_ctx_in->ovs_idl_txn) { > > + if (b_lport->lbinding->iface && qos_map && > > b_ctx_in->ovs_idl_txn) { > > get_qos_params(pb, qos_map); > > } > > } else { > > @@ -1136,16 +1094,19 @@ consider_vif_lport(const struct sbrec_port_binding > > *pb, > > vif_chassis); > > > > if (!lbinding) { > > - lbinding = local_binding_find(b_ctx_out->local_bindings, > > + lbinding = local_binding_find(&b_ctx_out->lbinding_data->bindings, > > pb->logical_port); > > } > > > > + struct binding_lport *b_lport = NULL; > > if (lbinding) { > > - lbinding->pb = pb; > > + struct shash *binding_lports = > > + &b_ctx_out->lbinding_data->lports; > > + b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > LP_VIF); > > } > > > > return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, > > - b_ctx_out, lbinding, qos_map); > > + b_ctx_out, b_lport, qos_map); > > } > > > > static bool > > @@ -1154,9 +1115,9 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct hmap *qos_map) > > { > > + struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > > struct local_binding *parent_lbinding; > > - parent_lbinding = local_binding_find(b_ctx_out->local_bindings, > > - pb->parent_port); > > + parent_lbinding = local_binding_find(local_bindings, pb->parent_port); > > > > if (!parent_lbinding) { > > /* There is no local_binding for parent port. Create it > > @@ -1171,40 +1132,35 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > * we want the these container ports also be claimed by the > > * chassis. > > * */ > > - parent_lbinding = local_binding_create(pb->parent_port, NULL, NULL, > > - BT_VIF); > > - local_binding_add(b_ctx_out->local_bindings, parent_lbinding); > > + parent_lbinding = local_binding_create(pb->parent_port, NULL); > > + local_binding_add(local_bindings, parent_lbinding); > > } > > > > - struct local_binding *container_lbinding = > > - local_binding_find_child(parent_lbinding, pb->logical_port); > > + struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > + struct binding_lport *container_b_lport = > > + local_binding_add_lport(binding_lports, parent_lbinding, pb, > > + LP_CONTAINER); > > > > - 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; > > - } > > + struct binding_lport *parent_b_lport = > > + binding_lport_find(binding_lports, pb->parent_port); > > > > - if (!parent_lbinding->pb) { > > - parent_lbinding->pb = lport_lookup_by_name( > > + if (!parent_b_lport || !parent_b_lport->pb) { > > + const struct sbrec_port_binding *parent_pb = lport_lookup_by_name( > > b_ctx_in->sbrec_port_binding_by_name, pb->parent_port); > > > > - if (parent_lbinding->pb) { > > + if (parent_pb && get_lport_type(parent_pb) == LP_VIF) { > > /* Its possible that the parent lport is not considered yet. > > * So call consider_vif_lport() to process it first. */ > > - consider_vif_lport(parent_lbinding->pb, b_ctx_in, b_ctx_out, > > + consider_vif_lport(parent_pb, b_ctx_in, b_ctx_out, > > parent_lbinding, qos_map); > > + parent_b_lport = binding_lport_find(binding_lports, > > + pb->parent_port); > > } 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, > > + * release the container lport, if it was bound earlier. */ > > + if (is_binding_lport_this_chassis(container_b_lport, > > + b_ctx_in->chassis_rec)) { > > + return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > b_ctx_out->tracked_dp_bindings); > > } > > > > @@ -1212,13 +1168,14 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > } > > } > > > > - const char *vif_chassis = smap_get(&parent_lbinding->pb->options, > > + ovs_assert(parent_b_lport && parent_b_lport->pb); > > + const char *vif_chassis = smap_get(&parent_b_lport->pb->options, > > "requested-chassis"); > > bool can_bind = can_bind_on_this_chassis(b_ctx_in->chassis_rec, > > vif_chassis); > > > > return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in, > > b_ctx_out, > > - container_lbinding, qos_map); > > + container_b_lport, qos_map); > > } > > > > static bool > > @@ -1227,46 +1184,47 @@ consider_virtual_lport(const struct > > sbrec_port_binding *pb, > > struct binding_ctx_out *b_ctx_out, > > struct hmap *qos_map) > > { > > + struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > > struct local_binding * parent_lbinding = > > Nit: Now that we're changing this code, we can also remove the extra > space after '*'. Ack. Done . > > > > - pb->virtual_parent ? local_binding_find(b_ctx_out->local_bindings, > > + pb->virtual_parent ? local_binding_find(local_bindings, > > pb->virtual_parent) > > : NULL; > > > > - if (parent_lbinding && !parent_lbinding->pb) { > > - parent_lbinding->pb = lport_lookup_by_name( > > - b_ctx_in->sbrec_port_binding_by_name, pb->virtual_parent); > > - > > - if (parent_lbinding->pb) { > > - /* Its possible that the parent lport is not considered yet. > > - * 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); > > - } > > - } > > - > > + struct binding_lport *virtual_b_lport = NULL; > > /* Unlike container lports, we don't have to create parent_lbinding if > > * it is NULL. This is because, if parent_lbinding is not present, it > > * means the virtual port can't bind in this chassis. > > * Note: pinctrl module binds the virtual lport when it sees ARP > > * packet from the parent lport. */ > > - struct local_binding *virtual_lbinding = NULL; > > - if (is_lbinding_this_chassis(parent_lbinding, b_ctx_in->chassis_rec)) { > > - virtual_lbinding = > > - local_binding_find_child(parent_lbinding, pb->logical_port); > > - 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; > > + if (parent_lbinding) { > > + struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > + > > + struct binding_lport *parent_b_lport = > > + binding_lport_find(binding_lports, pb->virtual_parent); > > + > > + if (!parent_b_lport || !parent_b_lport->pb) { > > + const struct sbrec_port_binding *parent_pb = > > lport_lookup_by_name( > > + b_ctx_in->sbrec_port_binding_by_name, pb->virtual_parent); > > + > > + if (parent_pb && get_lport_type(parent_pb) == LP_VIF) { > > + /* Its possible that the parent lport is not considered > > yet. > > + * So call consider_vif_lport() to process it first. */ > > + consider_vif_lport(parent_pb, b_ctx_in, b_ctx_out, > > + parent_lbinding, qos_map); > > + } > > + } > > + > > + parent_b_lport = local_binding_get_primary_lport(parent_lbinding); > > + if (is_binding_lport_this_chassis(parent_b_lport, > > + b_ctx_in->chassis_rec)) { > > + virtual_b_lport = > > + local_binding_add_lport(binding_lports, parent_lbinding, > > pb, > > + LP_VIRTUAL); > > } > > } > > > > return consider_vif_lport_(pb, true, NULL, b_ctx_in, b_ctx_out, > > - virtual_lbinding, qos_map); > > + virtual_b_lport, qos_map); > > } > > > > /* Considers either claiming the lport or releasing the lport > > @@ -1407,6 +1365,8 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > continue; > > } > > > > + struct shash *local_bindings = > > + &b_ctx_out->lbinding_data->bindings; > > for (j = 0; j < port_rec->n_interfaces; j++) { > > const struct ovsrec_interface *iface_rec; > > > > @@ -1416,11 +1376,10 @@ build_local_bindings(struct binding_ctx_in > > *b_ctx_in, > > > > if (iface_id && ofport > 0) { > > struct local_binding *lbinding = > > - local_binding_find(b_ctx_out->local_bindings, > > iface_id); > > + local_binding_find(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); > > + lbinding = local_binding_create(iface_id, iface_rec); > > + local_binding_add(local_bindings, lbinding); > > } else { > > static struct vlog_rate_limit rl = > > VLOG_RATE_LIMIT_INIT(1, 5); > > @@ -1431,7 +1390,6 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, > > "configuration on interface [%s]", > > lbinding->iface->name, iface_rec->name, > > iface_rec->name); > > - ovs_assert(lbinding->type == BT_VIF); > > } > > > > update_local_lports(iface_id, b_ctx_out); > > @@ -1494,11 +1452,11 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > > binding_ctx_out *b_ctx_out) > > break; > > > > case LP_VIF: > > - if (is_lport_container(pb)) { > > - consider_container_lport(pb, b_ctx_in, b_ctx_out, > > qos_map_ptr); > > - } else { > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, > > qos_map_ptr); > > - } > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL, qos_map_ptr); > > + break; > > + > > + case LP_CONTAINER: > > + consider_container_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > > break; > > > > case LP_VIRTUAL: > > @@ -1799,39 +1757,45 @@ consider_iface_claim(const struct ovsrec_interface > > *iface_rec, > > update_local_lports(iface_id, b_ctx_out); > > 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); > > + struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > > + struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > + struct local_binding *lbinding = local_binding_find(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); > > + lbinding = local_binding_create(iface_id, iface_rec); > > + local_binding_add(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")) { > > - lbinding->pb = NULL; > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > + const struct sbrec_port_binding *pb = NULL; > > + if (!b_lport) { > > + pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > > + lbinding->name); > > + if (pb && get_lport_type(pb) == LP_VIF) { > > + b_lport = local_binding_add_lport(binding_lports, lbinding, pb, > > + LP_VIF); > > } > > } > > > > - if (lbinding->pb) { > > - if (!consider_vif_lport(lbinding->pb, b_ctx_in, b_ctx_out, > > - lbinding, qos_map)) { > > - return false; > > - } > > + if (!b_lport) { > > + /* There is no binding lport for this local binding. */ > > + return true; > > + } > > + > > + if (!consider_vif_lport(b_lport->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, > > + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > + if (b_lport->type == LP_CONTAINER) { > > + if (!consider_container_lport(b_lport->pb, b_ctx_in, b_ctx_out, > > qos_map)) { > > return false; > > } > > @@ -1862,32 +1826,42 @@ consider_iface_release(const struct > > ovsrec_interface *iface_rec, > > struct binding_ctx_out *b_ctx_out) > > { > > struct local_binding *lbinding; > > - lbinding = local_binding_find(b_ctx_out->local_bindings, > > - iface_id); > > - if (is_lbinding_this_chassis(lbinding, b_ctx_in->chassis_rec)) { > > + struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > > + struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > + > > + lbinding = local_binding_find(local_bindings, iface_id); > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lbinding); > > + if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) { > > struct local_datapath *ld = > > get_local_datapath(b_ctx_out->local_datapaths, > > - lbinding->pb->datapath->tunnel_key); > > + b_lport->pb->datapath->tunnel_key); > > if (ld) { > > - remove_pb_from_local_datapath(lbinding->pb, > > - b_ctx_in->chassis_rec, > > - b_ctx_out, ld); > > + remove_pb_from_local_datapath(b_lport->pb, > > + b_ctx_in->chassis_rec, > > + b_ctx_out, ld); > > } > > > > - /* Note: release_local_binding() resets lbinding->pb and > > - * lbinding->iface. > > - * Cannot access these members of lbinding after this call. */ > > - if (!release_local_binding(b_ctx_in->chassis_rec, lbinding, > > - !b_ctx_in->ovnsb_idl_txn, > > - b_ctx_out->tracked_dp_bindings)) { > > - return false; > > + /* Release the primary binding lport and other children lports if > > + * any. */ > > + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > + if (!release_binding_lport(b_ctx_in->chassis_rec, b_lport, > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)) { > > + return false; > > + } > > } > > + > > + } > > + > > + if (lbinding) { > > + /* Clear the iface of the local binding. */ > > + lbinding->iface = NULL; > > } > > > > /* Check if the lbinding has children of type PB_CONTAINER. > > * If so, don't delete the local_binding. */ > > if (lbinding && !is_lbinding_container_parent(lbinding)) { > > - local_binding_delete(b_ctx_out->local_bindings, lbinding); > > + local_binding_delete(lbinding, local_bindings, binding_lports); > > } > > > > remove_local_lports(iface_id, b_ctx_out); > > @@ -2088,56 +2062,35 @@ handle_deleted_lport(const struct > > sbrec_port_binding *pb, > > } > > } > > > > -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) { > > - if (pb->virtual_parent) { > > - parent_lbinding = local_binding_find(b_ctx_out->local_bindings, > > - pb->virtual_parent); > > - } > > - } else { > > - if (pb->parent_port) { > > - 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) > > { > > - struct local_binding *lbinding = > > - get_lbinding_for_lport(pb, lport_type, b_ctx_out); > > + struct local_binding *lbinding = NULL; > > + bool bound = false; > > > > - 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, > > - b_ctx_out->tracked_dp_bindings)) { > > - return false; > > + struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; > > + struct binding_lport *b_lport = binding_lport_find(binding_lports, > > pb->logical_port); > > + if (b_lport) { > > + lbinding = b_lport->lbinding; > > + bound = is_binding_lport_this_chassis(b_lport, > > b_ctx_in->chassis_rec); > > + > > + /* Remove b_lport from local_binding. */ > > + binding_lport_delete(binding_lports, b_lport); > > + } > > + > > + if (bound && lbinding && lport_type == LP_VIF) { > > + /* We need to release the container/virtual binding lports (if > > any) if > > + * deleted 'pb' type is LP_VIF. */ > > + struct binding_lport *c_lport; > > + LIST_FOR_EACH (c_lport, list_node, &lbinding->binding_lports) { > > + if (!release_binding_lport(b_ctx_in->chassis_rec, c_lport, > > + !b_ctx_in->ovnsb_idl_txn, > > + b_ctx_out->tracked_dp_bindings)) { > > + return false; > > + } > > } > > } > > > > @@ -2147,18 +2100,8 @@ handle_deleted_vif_lport(const struct > > sbrec_port_binding *pb, > > * it from local_lports if there is a VIF entry. > > * consider_iface_release() takes care of removing from the > > local_lports > > * when the interface change happens. */ > > - if (is_lport_container(pb)) { > > + if (lport_type == LP_CONTAINER) { > > remove_local_lports(pb->logical_port, b_ctx_out); > > - > > - /* If the container port is removed we should also remove it from > > - * its parent's children set. > > - */ > > - if (lbinding) { > > - if (lbinding->parent) { > > - local_binding_delete_child(lbinding->parent, lbinding); > > - } > > - local_binding_destroy(lbinding); > > - } > > } > > > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > > @@ -2177,7 +2120,7 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > > > 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)) { > > + } else if (lport_type == LP_CONTAINER) { > > 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); > > @@ -2189,14 +2132,14 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > > > bool now_claimed = (pb->chassis == b_ctx_in->chassis_rec); > > > > - if (lport_type == LP_VIRTUAL || > > - (lport_type == LP_VIF && is_lport_container(pb)) || > > + if (lport_type == LP_VIRTUAL || lport_type == LP_CONTAINER || > > claimed == now_claimed) { > > return true; > > } > > > > - struct local_binding *lbinding = > > - local_binding_find(b_ctx_out->local_bindings, pb->logical_port); > > + struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > > + struct local_binding *lbinding = local_binding_find(local_bindings, > > + pb->logical_port); > > > > /* If the ovs port backing this binding previously was removed in the > > * meantime, we won't have a local_binding for it. > > @@ -2206,12 +2149,11 @@ handle_updated_vif_lport(const struct > > sbrec_port_binding *pb, > > return true; > > } > > > > - 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); > > + struct binding_lport *b_lport; > > + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > > + if (b_lport->type == LP_CONTAINER) { > > + handled = consider_container_lport(b_lport->pb, b_ctx_in, > > + b_ctx_out, qos_map); > > if (!handled) { > > return false; > > } > > @@ -2256,12 +2198,20 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > > > enum en_lport_type lport_type = get_lport_type(pb); > > > > + struct binding_lport *b_lport = > > + binding_lport_find(&b_ctx_out->lbinding_data->lports, > > + pb->logical_port); > > + if (b_lport) { > > + binding_lport_check_and_cleanup( > > + b_lport, lport_type, > > + &b_ctx_out->lbinding_data->lports); > > + /* Do not access b_lport after this. It may have been freed. > > */ > > + } > > + > > if (lport_type == LP_VIF) { > > - if (is_lport_container(pb)) { > > - shash_add(&deleted_container_pbs, pb->logical_port, pb); > > - } else { > > - shash_add(&deleted_vif_pbs, pb->logical_port, pb); > > - } > > + shash_add(&deleted_vif_pbs, pb->logical_port, pb); > > + } else if (lport_type == LP_CONTAINER) { > > + shash_add(&deleted_container_pbs, pb->logical_port, pb); > > } else if (lport_type == LP_VIRTUAL) { > > shash_add(&deleted_virtual_pbs, pb->logical_port, pb); > > } else { > > @@ -2272,7 +2222,7 @@ binding_handle_port_binding_changes(struct > > binding_ctx_in *b_ctx_in, > > struct shash_node *node; > > struct shash_node *node_next; > > SHASH_FOR_EACH_SAFE (node, node_next, &deleted_container_pbs) { > > - handled = handle_deleted_vif_lport(node->data, LP_VIF, b_ctx_in, > > + handled = handle_deleted_vif_lport(node->data, LP_CONTAINER, > > b_ctx_in, > > b_ctx_out); > > shash_delete(&deleted_container_pbs, node); > > if (!handled) { > > @@ -2326,12 +2276,33 @@ delete_done: > > > > enum en_lport_type lport_type = get_lport_type(pb); > > > > + struct binding_lport *b_lport = > > + binding_lport_find(&b_ctx_out->lbinding_data->lports, > > + pb->logical_port); > > + if (b_lport) { > > + ovs_assert(b_lport->pb == pb); > > + > > + if (b_lport->type != lport_type) { > > + b_lport->type = lport_type; > > + } > > + > > + if (b_lport->lbinding) { > > + handled = local_binding_handle_stale_binding_lports( > > + b_lport->lbinding, b_ctx_in, b_ctx_out, qos_map_ptr); > > + if (!handled) { > > + /* Backout from the handling. */ > > + break; > > + } > > + } > > + } > > + > > struct local_datapath *ld = > > get_local_datapath(b_ctx_out->local_datapaths, > > pb->datapath->tunnel_key); > > > > switch (lport_type) { > > case LP_VIF: > > + case LP_CONTAINER: > > case LP_VIRTUAL: > > handled = handle_updated_vif_lport(pb, lport_type, b_ctx_in, > > b_ctx_out, qos_map_ptr); > > @@ -2468,11 +2439,11 @@ binding_init(void) > > * available. > > */ > > void > > -binding_seqno_run(struct shash *local_bindings) > > +binding_seqno_run(struct local_binding_data *lbinding_data) > > { > > const char *iface_id; > > const char *iface_id_next; > > - > > + struct shash *local_bindings = &lbinding_data->bindings; > > SSET_FOR_EACH_SAFE (iface_id, iface_id_next, > > &binding_iface_released_set) { > > struct shash_node *lb_node = shash_find(local_bindings, iface_id); > > > > @@ -2508,16 +2479,17 @@ binding_seqno_run(struct shash *local_bindings) > > * If so, then this is a newly bound interface, make sure we reset > > the > > * Port_Binding 'up' field and the OVS Interface 'external-id'. > > */ > > - if (lb && lb->pb && lb->iface) { > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lb); > > + if (lb && b_lport && lb->iface) { > > new_ifaces = true; > > > > if (smap_get(&lb->iface->external_ids, OVN_INSTALLED_EXT_ID)) { > > ovsrec_interface_update_external_ids_delkey( > > lb->iface, OVN_INSTALLED_EXT_ID); > > } > > - if (lb->pb->n_up) { > > + if (b_lport->pb->n_up) { > > bool up = false; > > - sbrec_port_binding_set_up(lb->pb, &up, 1); > > + sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > } > > simap_put(&binding_iface_seqno_map, lb->name, new_seqno); > > } > > @@ -2542,12 +2514,13 @@ binding_seqno_run(struct shash *local_bindings) > > * available. > > */ > > void > > -binding_seqno_install(struct shash *local_bindings) > > +binding_seqno_install(struct local_binding_data *lbinding_data) > > { > > struct ofctrl_acked_seqnos *acked_seqnos = > > ofctrl_acked_seqnos_get(binding_seq_type_pb_cfg); > > struct simap_node *node; > > struct simap_node *node_next; > > + struct shash *local_bindings = &lbinding_data->bindings; > > > > SIMAP_FOR_EACH_SAFE (node, node_next, &binding_iface_seqno_map) { > > struct shash_node *lb_node = shash_find(local_bindings, > > node->name); > > @@ -2557,7 +2530,8 @@ binding_seqno_install(struct shash *local_bindings) > > } > > > > struct local_binding *lb = lb_node->data; > > - if (!lb->pb || !lb->iface) { > > + struct binding_lport *b_lport = > > local_binding_get_primary_lport(lb); > > + if (!b_lport || !lb->iface) { > > goto del_seqno; > > } > > > > @@ -2568,14 +2542,12 @@ binding_seqno_install(struct shash *local_bindings) > > ovsrec_interface_update_external_ids_setkey(lb->iface, > > OVN_INSTALLED_EXT_ID, > > "true"); > > - if (lb->pb->n_up) { > > + if (b_lport->pb->n_up) { > > bool up = true; > > > > - sbrec_port_binding_set_up(lb->pb, &up, 1); > > - struct shash_node *child_node; > > - SHASH_FOR_EACH (child_node, &lb->children) { > > - struct local_binding *lb_child = child_node->data; > > - sbrec_port_binding_set_up(lb_child->pb, &up, 1); > > + sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > + LIST_FOR_EACH (b_lport, list_node, &lb->binding_lports) { > > + sbrec_port_binding_set_up(b_lport->pb, &up, 1); > > } > > } > > > > @@ -2591,3 +2563,292 @@ binding_seqno_flush(void) > > { > > simap_clear(&binding_iface_seqno_map); > > } > > + > > +/* Static functions for local_lbindind and binding_lport. */ > > +static struct local_binding * > > +local_binding_create(const char *name, const struct ovsrec_interface > > *iface) > > +{ > > + struct local_binding *lbinding = xzalloc(sizeof *lbinding); > > + lbinding->name = xstrdup(name); > > + lbinding->iface = iface; > > + ovs_list_init(&lbinding->binding_lports); > > + > > + return lbinding; > > +} > > + > > +static struct local_binding * > > +local_binding_find(struct shash *local_bindings, const char *name) > > +{ > > + return shash_find_data(local_bindings, name); > > +} > > + > > +static void > > +local_binding_add(struct shash *local_bindings, struct local_binding > > *lbinding) > > +{ > > + shash_add(local_bindings, lbinding->name, lbinding); > > +} > > + > > +static void > > +local_binding_destroy(struct local_binding *lbinding, > > + struct shash *binding_lports) > > +{ > > + struct binding_lport *b_lport; > > + LIST_FOR_EACH_POP (b_lport, list_node, &lbinding->binding_lports) { > > + b_lport->lbinding = NULL; > > + binding_lport_delete(binding_lports, b_lport); > > + } > > + > > + free(lbinding->name); > > + free(lbinding); > > +} > > + > > +static void > > +local_binding_delete(struct local_binding *lbinding, > > + struct shash *local_bindings, > > + struct shash *binding_lports) > > +{ > > + shash_find_and_delete(local_bindings, lbinding->name); > > + local_binding_destroy(lbinding, binding_lports); > > +} > > + > > +/* Returns the primary binding lport if present in lbinding's > > + * binding lports list. A binding lport is considered primary > > + * if binding lport's type is LP_VIF and the name matches > > + * with the 'lbinding'. > > + */ > > +static struct binding_lport * > > +local_binding_get_primary_lport(struct local_binding *lbinding)() > > +{ > > + if (!lbinding) { > > + return NULL; > > + } > > + > > + if (!ovs_list_is_empty(&lbinding->binding_lports)) { > > + struct binding_lport *b_lport = NULL; > > + b_lport = CONTAINER_OF(ovs_list_front(&lbinding->binding_lports), > > + struct binding_lport, list_node); > > + > > + if (b_lport->type == LP_VIF && > > + !strcmp(lbinding->name, b_lport->name)) { > > Isn't it a bug if the first entry in binding_lports is of type LP_VIF > and doesn't have the same name as lbinding->name? This check is required for one scenario. If a container lport is updated to a normal VIF, then binding_handle_port_binding_changes() will first get the binding_lport for the pb. Then it will change the type. So the type will change form LP_CONTAINER to LP_VIF. It will then call local_binding_handle_stale_binding_lports(). Without the above !strcmp check, local_binding_get_primary_lport() will return the binding_lport. I tested by changing the above code to if(b_lport->type == LP_VIF) { ovs_assert(!strcmp(lbinding->name, b_lport->name); return b_lport; } The test case "container port changed from one parent to another" (added in this patch) fails with the above assertion. In the above mentioned case, the function local_binding_handle_stale_binding_lports() makes sure that binding_lport is cleaned up the changed port binding. > > > > + return b_lport; > > + } > > + } > > + > > + return NULL; > > +} > > + > > +static struct binding_lport * > > +local_binding_add_lport(struct shash *binding_lports, > > + struct local_binding *lbinding, > > + const struct sbrec_port_binding *pb, > > + enum en_lport_type b_type) > > +{ > > + struct binding_lport *b_lport = > > + binding_lport_find(binding_lports, pb->logical_port); > > + bool add_to_lport_list = false; > > + if (!b_lport) { > > + b_lport = binding_lport_create(pb, lbinding, b_type); > > + binding_lport_add(binding_lports, b_lport); > > + add_to_lport_list = true; > > + } else if (b_lport->lbinding != lbinding) { > > + add_to_lport_list = true; > > + if (!ovs_list_is_empty(&b_lport->list_node)) { > > + ovs_list_remove(&b_lport->list_node); > > + } > > + b_lport->lbinding = lbinding; > > + b_lport->type = b_type; > > + } > > + > > + if (add_to_lport_list) { > > + if (b_type == LP_VIF) { > > + ovs_list_push_front(&lbinding->binding_lports, > > &b_lport->list_node); > > + } else { > > + ovs_list_push_back(&lbinding->binding_lports, > > &b_lport->list_node); > > + } > > + } > > + > > + return b_lport; > > +} > > + > > +/* This function handles the stale binding lports of 'lbinding' > > + * if 'lbinding' doesn't have a primary binding lport. > > + */ > > Nit: the comment above might look prettier if wrapped at more than 64 > columns. Ack. Done. > > > +static bool > > +local_binding_handle_stale_binding_lports(struct local_binding *lbinding, > > + struct binding_ctx_in *b_ctx_in, > > + struct binding_ctx_out > > *b_ctx_out, > > + struct hmap *qos_map) > > +{ > > + /* Check if this lbinding has a primary binding_lport or not. */ > > + struct binding_lport *p_lport = > > local_binding_get_primary_lport(lbinding); > > + if (p_lport) { > > + /* Nothing to be done. */ > > + return true; > > + } > > + > > + bool handled = true; > > + struct binding_lport *b_lport, *next; > > + const struct sbrec_port_binding *pb; > > + LIST_FOR_EACH_SAFE (b_lport, next, list_node, > > &lbinding->binding_lports) { > > + /* Get the lport type again from the pb. Its possible that the > > + * pb type can change. */ > > Nit: s/can change/has changed/ > > > + enum en_lport_type pb_lport_type = get_lport_type(b_lport->pb); > > + if (b_lport->type == LP_VIRTUAL && pb_lport_type == LP_VIRTUAL) { > > + pb = b_lport->pb; > > + binding_lport_delete(&b_ctx_out->lbinding_data->lports, > > + b_lport); > > + handled = consider_virtual_lport(pb, b_ctx_in, b_ctx_out, > > qos_map); > > + } else if (b_lport->type == LP_CONTAINER && > > + pb_lport_type == LP_CONTAINER) { > > + /* For container lport, binding_lport is preserved so that when > > + * the parent port is created, it can be considered. > > + * consider_container_lport() creates the binding_lport for > > the parent > > + * port (with iface set to NULL). */ > > + handled = consider_container_lport(b_lport->pb, b_ctx_in, > > b_ctx_out, qos_map); > > + } else { > > + /* This can happen when the lport type changes from one type > > + * to another. Eg. from normal lport to external. Release the > > + * lport if it was claimed earlier and delete the b_lport. */ > > + handled = release_binding_lport(b_ctx_in->chassis_rec, b_lport, > > + !b_ctx_in->ovnsb_idl_txn, > > + > > b_ctx_out->tracked_dp_bindings); > > + binding_lport_delete(&b_ctx_out->lbinding_data->lports, > > + b_lport); > > + } > > + > > + if (!handled) { > > + return false; > > + } > > + } > > + > > + return handled; > > +} > > + > > +static struct binding_lport * > > +binding_lport_create(const struct sbrec_port_binding *pb, > > + struct local_binding *lbinding, > > + enum en_lport_type type) > > +{ > > + struct binding_lport *b_lport = xzalloc(sizeof *b_lport); > > + b_lport->name = xstrdup(pb->logical_port); > > + b_lport->pb = pb; > > + b_lport->type = type; > > + b_lport->lbinding = lbinding; > > + ovs_list_init(&b_lport->list_node); > > + > > + return b_lport; > > +} > > + > > +static void > > +binding_lport_add(struct shash *binding_lports, struct binding_lport > > *b_lport) > > +{ > > + shash_add(binding_lports, b_lport->pb->logical_port, b_lport); > > +} > > + > > +static struct binding_lport * > > +binding_lport_find(struct shash *binding_lports, const char *lport_name) > > +{ > > + if (!lport_name) { > > + return NULL; > > + } > > + > > + return shash_find_data(binding_lports, lport_name); > > +} > > + > > +static void > > +binding_lport_destroy(struct binding_lport *b_lport) > > +{ > > + if (!ovs_list_is_empty(&b_lport->list_node)) { > > + ovs_list_remove(&b_lport->list_node); > > + } > > + > > + free(b_lport->name); > > + free(b_lport); > > +} > > + > > +static void > > +binding_lport_delete(struct shash *binding_lports, > > + struct binding_lport *b_lport) > > +{ > > + shash_find_and_delete(binding_lports, b_lport->name); > > + binding_lport_destroy(b_lport); > > +} > > + > > + > > +static const struct sbrec_port_binding * > > +binding_lport_get_parent_pb(struct binding_lport *b_lport) > > +{ > > + if (!b_lport) { > > + return NULL; > > + } > > + > > + if (b_lport->type == LP_VIF) { > > + return NULL; > > + } > > + > > + struct local_binding *lbinding = b_lport->lbinding; > > + ovs_assert(lbinding); > > + > > + struct binding_lport *parent_b_lport = > > + local_binding_get_primary_lport(lbinding); > > + > > + return parent_b_lport ? parent_b_lport->pb : NULL; > > +} > > + > > +static struct binding_lport * > > +binding_lport_check_and_cleanup(struct binding_lport *b_lport, > > + enum en_lport_type lport_type, > > + struct shash *binding_lports) > > +{ > > + if (b_lport->type != lport_type) { > > + b_lport->type = lport_type; > > I'm not sure I understand why we need to change b_lport->type here. I > think this would also need to be added in a comment before the function > definition. I get your point. I moved the b_lport->type update in the function binding_handle_port_binding_changes(). For the deleted port bindings, we will first get the binding_lport and then update the type if it is changed and then call binding_lport_check_and_cleanup(). > > > + } > > + > > + bool cleanup_blport = false; > > + > > + if (!b_lport->lbinding) { > > + cleanup_blport = true; > > + goto cleanup; > > + } > > + > > + switch (b_lport->type) { > > + case LP_VIF: > > + if (strcmp(b_lport->name, b_lport->lbinding->name)) { > > + cleanup_blport = true; > > + } > > + break; > > + > > + case LP_CONTAINER: > > + if (strcmp(b_lport->pb->parent_port, b_lport->lbinding->name)) { > > + cleanup_blport = true; > > + } > > + break; > > + > > + case LP_VIRTUAL: > > + if (!b_lport->pb->virtual_parent || > > + strcmp(b_lport->pb->virtual_parent, b_lport->lbinding->name)) { > > + cleanup_blport = true; > > + } > > + break; > > + > > + case LP_PATCH: > > + case LP_LOCALPORT: > > + case LP_VTEP: > > + case LP_L2GATEWAY: > > + case LP_L3GATEWAY: > > + case LP_CHASSISREDIRECT: > > + case LP_EXTERNAL: > > + case LP_LOCALNET: > > + case LP_REMOTE: > > + case LP_UNKNOWN: > > + cleanup_blport = true; > > + } > > + > > +cleanup: > > + if (cleanup_blport) { > > + binding_lport_delete(binding_lports, b_lport); > > + return NULL; > > + } > > + > > + return b_lport; > > +} > > diff --git a/controller/binding.h b/controller/binding.h > > index c9ebef4b17..b8a1a5d796 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -56,7 +56,7 @@ struct binding_ctx_in { > > > > struct binding_ctx_out { > > struct hmap *local_datapaths; > > - struct shash *local_bindings; > > + struct local_binding_data *lbinding_data; > > > > /* sset of (potential) local lports. */ > > struct sset *local_lports; > > @@ -86,28 +86,16 @@ struct binding_ctx_out { > > struct hmap *tracked_dp_bindings; > > }; > > > > -enum local_binding_type { > > - BT_VIF, > > - BT_CONTAINER, > > - BT_VIRTUAL > > +struct local_binding_data { > > + struct shash bindings; > > + struct shash lports; > > }; > > > > -struct local_binding { > > - char *name; > > - enum local_binding_type type; > > - const struct ovsrec_interface *iface; > > - const struct sbrec_port_binding *pb; > > - > > - /* shash of 'struct local_binding' representing children. */ > > - struct shash children; > > - struct local_binding *parent; > > -}; > > +void local_binding_data_init(struct local_binding_data *); > > +void local_binding_data_destroy(struct local_binding_data *); > > > > -static inline struct local_binding * > > -local_binding_find(struct shash *local_bindings, const char *name) > > -{ > > - return shash_find_data(local_bindings, name); > > -} > > +const struct sbrec_port_binding *local_binding_get_primary_pb( > > + struct shash *local_bindings, const char *name); > > > > /* Represents a tracked binding logical port. */ > > struct tracked_binding_lport { > > @@ -137,7 +125,7 @@ bool binding_handle_port_binding_changes(struct > > binding_ctx_in *, > > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > > > > void binding_init(void); > > -void binding_seqno_run(struct shash *local_bindings); > > -void binding_seqno_install(struct shash *local_bindings); > > +void binding_seqno_run(struct local_binding_data *lbinding_data); > > +void binding_seqno_install(struct local_binding_data *lbinding_data); > > void binding_seqno_flush(void); > > #endif /* controller/binding.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 5dd643f52b..7e00c2e270 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1182,8 +1182,7 @@ struct ed_type_runtime_data { > > /* Contains "struct local_datapath" nodes. */ > > struct hmap local_datapaths; > > > > - /* Contains "struct local_binding" nodes. */ > > - struct shash local_bindings; > > + struct local_binding_data lbinding_data; > > > > /* Contains the name of each logical port resident on the local > > * hypervisor. These logical ports include the VIFs (and their child > > @@ -1222,9 +1221,9 @@ struct ed_type_runtime_data { > > * | | Interface and Port Binding changes store the > > | > > * | @tracked_dp_bindings | changed datapaths (datapaths added/removed > > from | > > * | | local_datapaths) and changed port bindings > > | > > - * | | (added/updated/deleted in 'local_bindings'). > > | > > + * | | (added/updated/deleted in 'lbinding_data'). > > | > > * | | So any changes to the runtime data - > > | > > - * | | local_datapaths and local_bindings is captured > > | > > + * | | local_datapaths and lbinding_data is captured > > | > > * | | here. > > | > > * > > ------------------------------------------------------------------------ > > * | | This is a bool which represents if the runtime > > | > > @@ -1251,7 +1250,7 @@ struct ed_type_runtime_data { > > * > > * --------------------------------------------------------------------- > > * | local_datapaths | The changes to these runtime data is captured in | > > - * | local_bindings | the @tracked_dp_bindings indirectly and hence it | > > + * | lbinding_data | the @tracked_dp_bindings indirectly and hence it | > > * | local_lport_ids | is not tracked explicitly. | > > * --------------------------------------------------------------------- > > * | local_iface_ids | This is used internally within the runtime data | > > @@ -1294,7 +1293,7 @@ en_runtime_data_init(struct engine_node *node > > OVS_UNUSED, > > sset_init(&data->active_tunnels); > > sset_init(&data->egress_ifaces); > > smap_init(&data->local_iface_ids); > > - local_bindings_init(&data->local_bindings); > > + local_binding_data_init(&data->lbinding_data); > > > > /* Init the tracked data. */ > > hmap_init(&data->tracked_dp_bindings); > > @@ -1322,7 +1321,7 @@ en_runtime_data_cleanup(void *data) > > free(cur_node); > > } > > hmap_destroy(&rt_data->local_datapaths); > > - local_bindings_destroy(&rt_data->local_bindings); > > + local_binding_data_destroy(&rt_data->lbinding_data); > > hmapx_destroy(&rt_data->ct_updated_datapaths); > > } > > > > @@ -1405,7 +1404,7 @@ init_binding_ctx(struct engine_node *node, > > b_ctx_out->local_lport_ids_changed = false; > > b_ctx_out->non_vif_ports_changed = false; > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces; > > - b_ctx_out->local_bindings = &rt_data->local_bindings; > > + b_ctx_out->lbinding_data = &rt_data->lbinding_data; > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids; > > b_ctx_out->tracked_dp_bindings = NULL; > > b_ctx_out->local_lports_changed = NULL; > > @@ -1449,7 +1448,7 @@ en_runtime_data_run(struct engine_node *node, void > > *data) > > free(cur_node); > > } > > hmap_clear(local_datapaths); > > - local_bindings_destroy(&rt_data->local_bindings); > > + local_binding_data_destroy(&rt_data->lbinding_data); > > sset_destroy(local_lports); > > sset_destroy(local_lport_ids); > > sset_destroy(active_tunnels); > > @@ -1460,7 +1459,7 @@ en_runtime_data_run(struct engine_node *node, void > > *data) > > sset_init(active_tunnels); > > sset_init(&rt_data->egress_ifaces); > > smap_init(&rt_data->local_iface_ids); > > - local_bindings_init(&rt_data->local_bindings); > > + local_binding_data_init(&rt_data->lbinding_data); > > hmapx_clear(&rt_data->ct_updated_datapaths); > > } > > > > @@ -1822,7 +1821,7 @@ static void init_physical_ctx(struct engine_node > > *node, > > p_ctx->local_lports = &rt_data->local_lports; > > p_ctx->ct_zones = ct_zones; > > p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > > - p_ctx->local_bindings = &rt_data->local_bindings; > > + p_ctx->local_bindings = &rt_data->lbinding_data.bindings; > > p_ctx->ct_updated_datapaths = &rt_data->ct_updated_datapaths; > > } > > > > @@ -2955,7 +2954,7 @@ main(int argc, char *argv[]) > > ovnsb_cond_seqno, > > ovnsb_expected_cond_seqno)); > > if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) { > > - binding_seqno_run(&runtime_data->local_bindings); > > + binding_seqno_run(&runtime_data->lbinding_data); > > } > > > > flow_output_data = engine_get_data(&en_flow_output); > > @@ -2968,7 +2967,7 @@ main(int argc, char *argv[]) > > } > > ofctrl_seqno_run(ofctrl_get_cur_cfg()); > > if (runtime_data && ovs_idl_txn && ovnsb_idl_txn) { > > - > > binding_seqno_install(&runtime_data->local_bindings); > > + > > binding_seqno_install(&runtime_data->lbinding_data); > > } > > } > > > > diff --git a/controller/physical.c b/controller/physical.c > > index fa5d0d692d..874d1ee270 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -1839,20 +1839,19 @@ physical_handle_ovs_iface_changes(struct > > physical_ctx *p_ctx, > > continue; > > } > > > > - const struct local_binding *lb = > > - local_binding_find(p_ctx->local_bindings, iface_id); > > - > > - if (!lb || !lb->pb) { > > + const struct sbrec_port_binding *lb_pb = > > + local_binding_get_primary_pb(p_ctx->local_bindings, iface_id); > > + if (!lb_pb) { > > continue; > > } > > > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; > > if (ovsrec_interface_is_deleted(iface_rec)) { > > - ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid); > > + ofctrl_remove_flows(flow_table, &lb_pb->header_.uuid); > > simap_find_and_delete(&localvif_to_ofport, iface_id); > > } else { > > if (!ovsrec_interface_is_new(iface_rec)) { > > - ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid); > > + ofctrl_remove_flows(flow_table, &lb_pb->header_.uuid); > > } > > > > simap_put(&localvif_to_ofport, iface_id, ofport); > > @@ -1860,7 +1859,7 @@ physical_handle_ovs_iface_changes(struct physical_ctx > > *p_ctx, > > p_ctx->mff_ovn_geneve, p_ctx->ct_zones, > > p_ctx->active_tunnels, > > p_ctx->local_datapaths, > > - lb->pb, p_ctx->chassis, > > + lb_pb, p_ctx->chassis, > > flow_table, &ofpacts); > > } > > } > > diff --git a/tests/ovn.at b/tests/ovn.at > > index b751d6db2e..9a3b5d8225 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -25161,3 +25161,307 @@ AT_CHECK([cat hv2_offlows_table72.txt | grep -v > > NXST], [1], [dnl > > OVN_CLEANUP([hv1], [hv2]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- container port changed to normal port and then deleted]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int vm1 > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl lsp-add ls vm-cont vm1 1 > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > > + > > +wait_for_ports_up > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl clear logical_switch_port vm-cont parent_name > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=foo > > +check ovn-nbctl lsp-del vm-cont > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not asserted. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > +wait_column "false" nb:Logical_Switch_Port up name=vm1 > > + > > +check ovn-nbctl lsp-add ls vm-cont1 vm1 1 > > +check ovn-nbctl lsp-add ls vm-cont2 vm1 2 > > + > > +check ovn-nbctl --wait=sb lsp-del vm1 > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl clear logical_switch_port vm-cont1 parent_name > > +check ovn-nbctl clear logical_switch_port vm-cont2 parent_name > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +check ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not crashed. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl set logical_switch_port vm-cont1 parent_name=vm1 > > +check ovn-nbctl --wait=sb set logical_switch_port vm-cont2 parent_name=vm1 > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > > + > > +wait_for_ports_up > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl --wait=sb lsp-del vm1 > > +check ovn-nbctl clear logical_switch_port vm-cont1 parent_name > > +check ovn-nbctl --wait=sb clear logical_switch_port vm-cont2 parent_name > > +check ovn-nbctl lsp-del vm-cont1 > > +check ovn-nbctl --wait=sb lsp-del vm-cont2 > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +check ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not crashed. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl lsp-add ls vm-cont1 vm1 1 > > +check ovn-nbctl lsp-add ls vm-cont2 vm1 2 > > + > > +wait_for_ports_up > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl clear logical_switch_port vm-cont1 parent_name > > +check ovn-nbctl --wait=sb clear logical_switch_port vm-cont2 parent_name > > +check ovn-nbctl lsp-del vm-cont1 > > +check ovn-nbctl lsp-del vm-cont2 > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +check ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not crashed. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > +check ovn-nbctl lsp-add ls vm-cont1 vm1 1 > > +check ovn-nbctl lsp-add ls vm-cont2 vm1 2 > > + > > +wait_for_ports_up > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl clear logical_switch_port vm-cont1 parent_name > > +check ovn-nbctl --wait=sb clear logical_switch_port vm-cont2 parent_name > > + > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=foo > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +wait_column "false" nb:Logical_Switch_Port up name=vm1 > > +wait_column "false" nb:Logical_Switch_Port up name=vm-cont1 > > +wait_column "false" nb:Logical_Switch_Port up name=vm-cont2 > > + > > +check ovn-nbctl set logical_switch_port vm-cont1 parent_name=vm1 > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > > +check ovn-nbctl --wait=sb set logical_switch_port vm-cont2 parent_name=vm1 > > + > > +wait_for_ports_up > > + > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl clear logical_switch_port vm-cont1 parent_name > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont1 > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +wait_column "false" nb:Logical_Switch_Port up name=vm1 > > +wait_column "true" nb:Logical_Switch_Port up name=vm-cont1 > > +wait_column "false" nb:Logical_Switch_Port up name=vm-cont2 > > + > > +check ovn-nbctl --wait=sb set logical_switch_port vm-cont2 > > parent_name=vm-cont1 > > +check ovn-nbctl --wait=sb set logical_switch_port vm1 parent_name=vm-cont1 > > + > > +wait_for_ports_up > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- container port changed from one parent to another]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int vm1 -- set interface vm1 ofport-request=1 > > +ovs-vsctl -- add-port br-int vm2 -- set interface vm1 ofport-request=2 > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl lsp-add ls vm1-cont vm1 1 > > +check ovn-nbctl lsp-add ls vm2 > > +check ovn-nbctl lsp-add ls vm2-cont vm2 2 > > + > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > > +check as hv1 ovs-vsctl set Interface vm2 external_ids:iface-id=vm2 > > + > > +wait_for_ports_up > > + > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c dl_vlan=1], > > [0], [dnl > > +1 > > +]) > > + > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c dl_vlan=2], > > [0], [dnl > > +1 > > +]) > > + > > +# change the parent of vm1-cont to vm2. > > +as hv1 ovn-appctl -t ovn-controller vlog/set dbg > > +check ovn-nbctl --wait=sb set logical_switch_port vm1-cont parent_name=vm2 > > \ > > +-- set logical_switch_port vm1-cont tag_request=3 > > + > > +wait_for_ports_up > > + > > +check ovn-nbctl --wait=hv sync > > + > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c dl_vlan=1], > > [1], [dnl > > +0 > > +]) > > + > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c dl_vlan=2], > > [0], [dnl > > +1 > > +]) > > + > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep -c dl_vlan=3], > > [0], [dnl > > +1 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- container port use-after-free test]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int vm1 > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl lsp-add ls vm-cont vm1 1 > > +check ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > > +check ovn-nbctl clear logical_switch_port vm-cont parent_name > > +check ovs-vsctl set Interface vm1 external_ids:iface-id=foo > > +check ovn-nbctl lsp-del vm-cont > > +check ovn-nbctl ls-del ls > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl lsp-add ls vm1 > > +check ovn-nbctl lsp-add ls vm-cont vm1 1 > > +check ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl clear logical_switch_port vm-cont parent_name > > +check ovn-nbctl lsp-del vm-cont > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=foo > > + > > +ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not asserted. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > +wait_column "false" nb:Logical_Switch_Port up name=vm1 > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > + > > +# Test that OVS.external_ids:iface-id doesn't affect non-VIF port bindings. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- Non-VIF ports incremental processing]) > > +ovn_start > > + > > +net_add n1 > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.10 > > + > > +check ovn-nbctl ls-add ls1 -- lsp-add ls1 lsp1 > > + > > +as hv1 > > +check ovs-vsctl \ > > + -- add-port br-int vif1 \ > > + -- set Interface vif1 external_ids:iface-id=lsp1 > > + > > +# ovn-controller should bind the interface. > > +wait_for_ports_up > > +hv_uuid=$(fetch_column Chassis _uuid name=hv1) > > +check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1 > > + > > +# Change the port type to router, ovn-controller should release it. > > +check ovn-nbctl --wait=hv lsp-set-type lsp1 router > > +#check_column "" Port_Binding chassis logical_port=lsp1 > > + > > +# Clear port type, ovn-controller should rebind it. > > +check ovn-nbctl --wait=hv lsp-set-type lsp1 '' > > +check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1 > > + > > +# Change the port type to localnet, ovn-controller should release it. > > +check ovn-nbctl --wait=hv lsp-set-type lsp1 localnet > > +check_column "" Port_Binding chassis logical_port=lsp1 > > + > > +# Clear port type, ovn-controller should rebind it. > > +check ovn-nbctl --wait=hv lsp-set-type lsp1 '' > > +check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1 > > + > > +# Change the port type to localport, ovn-controller should release it. > > +check ovn-nbctl --wait=hv lsp-set-type lsp1 localport > > +check_column "" Port_Binding chassis logical_port=lsp1 > > + > > +# Clear port type, ovn-controller should rebind it. > > +check ovn-nbctl --wait=hv lsp-set-type lsp1 '' > > +check_column "$hv_uuid" Port_Binding chassis logical_port=lsp1 > > + > > +# Change the port type to localnet and then delete it. > > +# ovn-controller should handle this properly. > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl --wait=sb lsp-set-type lsp1 localport > > +check ovn-nbctl --wait=sb lsp-del lsp1 > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +check ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not asserted. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > +check ovn-nbctl lsp-add ls1 lsp1 > > +wait_for_ports_up > > + > > +# Change the port type to virtual and then delete it. > > +# ovn-controller should handle this properly. > > +check as hv1 ovn-appctl -t ovn-controller debug/pause > > +check ovn-nbctl --wait=sb lsp-set-type lsp1 virtual > > +check ovn-nbctl --wait=sb lsp-del lsp1 > > +check as hv1 ovn-appctl -t ovn-controller debug/resume > > + > > +check ovn-nbctl --wait=hv sync > > + > > +# Make sure that ovn-controller has not asserted. > > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > > + > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > > > Would it be useful to add more tests that cover changing port binding > type for all possible combinations? I've added a test case to cover this. Request to check out v5. Thanks Numan > > Regards, > Dumitru > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
