[ovs-dev] [PATCH ovn v4] binding: Fix the crashes seen when port binding type changes.
Numan Siddique
numans at ovn.org
Mon Mar 29 18:29:23 UTC 2021
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 <dceara at redhat.com> wrote:
>
> On 3/23/21 4:55 PM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > 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 <dceara at redhat.com>
> > 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 <dceara at redhat.com>
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
>
> 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list