[ovs-dev] [PATCH ovn v5] binding: Fix the crashes seen when port binding type changes.
Dumitru Ceara
dceara at redhat.com
Wed Mar 31 11:32:16 UTC 2021
On 3/29/21 3:21 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.
>
> A new debug unixctl command is added - debug/dump-local-bindings,
> which dumps the local bindings stored by the ovn-controller. This
> command is also used in the test cases to validate that ovn-controller
> maintains proper local bindings.
>
> 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>
> [dceara at redhat.com contributed to the test cases which helped in reproducing the crashes.]
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
Hi Numan,
I think the changes are OK, I hope we didn't miss any cases though. As
Mark mentioned on an older revision, the additional tests do give a
certain level of confidence.
I'd recommend removing the "co-authored-by" tag above as you've been
adding and improving the tests a lot since we first hit this issue and
since v1 was posted.
That would also allow me to:
Acked-by: Dumitru Ceara <dceara at redhat.com>
I do have some minor nits/comments below, please have a look at them
before pushing this change.
Regards,
Dumitru
> v4 -> v5
> ----
> * Addressed review comments from Mark and Dumitru.
> * Added a new debug unixctl command - debug/dump-local-bindings
> * Added more test cases.
>
> 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 | 1114 +++++++++++++++++++++++------------
> controller/binding.h | 36 +-
> controller/ovn-controller.c | 44 +-
> controller/physical.c | 13 +-
> tests/ovn-controller.at | 80 +++
> tests/ovn.at | 640 ++++++++++++++++++++
> 6 files changed, 1517 insertions(+), 410 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 4e6c756969..b4dc6982c9 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,180 @@ 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
> - * - 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.
> - *
> + * struct local_binding has 3 main fields:
> + * - name : 'external_ids:iface-id' of the OVS interface (key).
> + * - OVS interface row object.
> + * - 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 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 interface that has external_ids:iface-id configured.
> *
> - * - 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.
> */
> +struct local_binding {
> + char *name;
> + const struct ovsrec_interface *iface;
> + struct ovs_list binding_lports;
> +};
>
> -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;
> -}
> -
> -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 *, struct shash *b_lports);
> +
> +static char *get_lport_type_str(enum en_lport_type lport_type);
>
> 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) {
> +
> + 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);
> + }
> +
> + SHASH_FOR_EACH_SAFE (node, next, &lbinding_data->bindings) {
> struct local_binding *lbinding = node->data;
> - local_binding_destroy(lbinding);
> - shash_delete(local_bindings, node);
> + local_binding_destroy(lbinding, &lbinding_data->lports);
> + shash_delete(&lbinding_data->bindings, node);
> }
>
> - shash_destroy(local_bindings);
> + shash_destroy(&lbinding_data->lports);
> + shash_destroy(&lbinding_data->bindings);
> }
>
> -static
> -void local_binding_delete(struct shash *local_bindings,
> - struct local_binding *lbinding)
> +const struct sbrec_port_binding *
> +local_binding_get_primary_pb(struct shash *local_bindings, const char *pb_name)
> {
> - shash_find_and_delete(local_bindings, lbinding->name);
> - local_binding_destroy(lbinding);
> -}
> + struct local_binding *lbinding =
> + local_binding_find(local_bindings, pb_name);
> + struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding);
>
> -static void
> -local_binding_add_child(struct local_binding *lbinding,
> - struct local_binding *child)
> -{
> - local_binding_add(&lbinding->children, child);
> - child->parent = lbinding;
> + return b_lport ? b_lport->pb : NULL;
> }
>
> -static struct local_binding *
> -local_binding_find_child(struct local_binding *lbinding,
> - const char *child_name)
> +void
> +binding_dump_local_bindings(struct local_binding_data *lbinding_data,
> + struct ds *out_data)
> {
> - return local_binding_find(&lbinding->children, child_name);
> -}
> + const struct shash_node **nodes;
> + size_t i, n;
>
> -static void
> -local_binding_delete_child(struct local_binding *lbinding,
> - struct local_binding *child)
> -{
> - shash_find_and_delete(&lbinding->children, child->name);
> + nodes = shash_sort(&lbinding_data->bindings);
> + n = shash_count(&lbinding_data->bindings);
Nit: size_t n = shash_count(&lbinding_data->bindings);
> +
> + ds_put_cstr(out_data, "Local bindings:\n");
> + for (i = 0; i < n; i++) {
Nit: 'i' could be defined here instead of at the top of the function.
> + const struct shash_node *node = nodes[i];
> + struct local_binding *lbinding = node->data;
> + size_t num_lports = ovs_list_size(&lbinding->binding_lports);
> + ds_put_format(out_data, "name: [%s], OVS interface name : [%s], "
> + "num binding lports : [%"PRIuSIZE"]\n",
> + lbinding->name,
> + lbinding->iface ? lbinding->iface->name : "NULL",
> + num_lports);
> +
> + if (num_lports) {
> + struct binding_lport *b_lport;
> + size_t j = 0;
> + size_t num_c_lports = 0;
> + struct shash child_lports = SHASH_INITIALIZER(&child_lports);
> + bool primary_lport = false;
Tiny nit: we could use reverse xmas tree here :)
Also, we don't need num_c_lports, we can just use shash_is_empty() and
shash_count().
> + LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) {
> + if (j == 0 && b_lport->type == LP_VIF) {
> + ds_put_format(out_data, "primary lport : [%s]\n",
> + b_lport->name);
> + primary_lport = true;
Would it make the code simpler if 'primary_lport' would actually be a
'struct binding_lport *' pointing to the first list element, if that's
of type LP_VIF? We could then display the primary port information in a
single place, below, where we now check "if (!primary_lport)".
> + } else {
> + shash_add(&child_lports, b_lport->name, b_lport);
> + num_c_lports++;
> + }
> + j++;
> + }
> +
> + if (!primary_lport) {
> + ds_put_format(out_data, "no primary lport\n");
> + }
> + if (num_c_lports) {
> + const struct shash_node **c_nodes =
> + shash_sort(&child_lports);
> + for (j = 0; j < num_c_lports; j++) {
> + b_lport = c_nodes[j]->data;
> + ds_put_format(out_data, "child lport[%"PRIuSIZE"] : [%s], "
> + "type : [%s]\n", j + 1, b_lport->name,
> + get_lport_type_str(b_lport->type));
> + }
> + free(c_nodes);
> + }
> + shash_destroy(&child_lports);
> + }
> +
> + ds_put_cstr(out_data, "----------------------------------------\n");
> + }
> +
> + free(nodes);
> }
>
> static bool
> @@ -744,12 +807,6 @@ is_lport_vif(const struct sbrec_port_binding *pb)
> return !pb->type[0];
> }
>
> -static bool
> -is_lport_container(const struct sbrec_port_binding *pb)
> -{
> - return is_lport_vif(pb) && pb->parent_port && pb->parent_port[0];
> -}
> -
> static struct tracked_binding_datapath *
> tracked_binding_datapath_create(const struct sbrec_datapath_binding *dp,
> bool is_new,
> @@ -818,26 +875,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 (pb->parent_port && pb->parent_port[0]) {
> + return LP_CONTAINER;
> + }
> return LP_VIF;
> } else if (!strcmp(pb->type, "patch")) {
> return LP_PATCH;
> @@ -864,6 +908,41 @@ get_lport_type(const struct sbrec_port_binding *pb)
> return LP_UNKNOWN;
> }
>
> +static char *
> +get_lport_type_str(enum en_lport_type lport_type)
> +{
> + switch (lport_type) {
> + case LP_VIF:
> + return "VIF";
> + case LP_CONTAINER:
> + return "CONTAINER";
> + case LP_VIRTUAL:
> + return "VIRTUAL";
> + case LP_PATCH:
> + return "PATCH";
> + case LP_CHASSISREDIRECT:
> + return "CHASSISREDIRECT";
> + case LP_L3GATEWAY:
> + return "L3GATEWAT";
> + case LP_LOCALNET:
> + return "PATCH";
> + case LP_LOCALPORT:
> + return "LOCALPORT";
> + case LP_L2GATEWAY:
> + return "L2GATEWAY";
> + case LP_EXTERNAL:
> + return "EXTERNAL";
> + case LP_REMOTE:
> + return "REMOTE";
> + case LP_VTEP:
> + return "VTEP";
> + case LP_UNKNOWN:
> + return "UNKNOWN";
> + }
> +
> + OVS_NOT_REACHED();
> +}
> +
> /* For newly claimed ports, if 'notify_up' is 'false':
> * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'.
> * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for
> @@ -991,14 +1070,15 @@ 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 && chassis &&
> + b_lport->pb->chassis == chassis);
> }
>
> static bool
> @@ -1010,15 +1090,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 +1106,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);
> +
> 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 +1150,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 +1188,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 +1209,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,54 +1226,61 @@ 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(
> + bool can_consider_c_lport = true;
> + 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,
> - b_ctx_out->tracked_dp_bindings);
> - }
> + /* The parent lport doesn't exist. Cannot consider the container
> + * lport for binding. */
> + can_consider_c_lport = false;
> + }
> + }
>
> - return true;
> + if (parent_b_lport && parent_b_lport->type != LP_VIF) {
> + can_consider_c_lport = false;
> + }
> +
> + if (!can_consider_c_lport) {
> + /* Call release_lport() to 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);
> }
> +
> + return true;
> }
>
> - 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 +1289,47 @@ consider_virtual_lport(const struct sbrec_port_binding *pb,
> struct binding_ctx_out *b_ctx_out,
> struct hmap *qos_map)
> {
> - struct local_binding * parent_lbinding =
> - pb->virtual_parent ? local_binding_find(b_ctx_out->local_bindings,
> + struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings;
> + struct local_binding *parent_lbinding =
> + 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 +1470,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 +1481,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 +1495,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 +1557,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 +1862,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;
> }
>
> +
Nit: extra line not needed.
> /* 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 +1931,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 +2167,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 +2205,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 +2225,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 +2237,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 +2254,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 +2303,25 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in,
>
> enum en_lport_type lport_type = get_lport_type(pb);
>
> - 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);
> + struct binding_lport *b_lport =
> + binding_lport_find(&b_ctx_out->lbinding_data->lports,
> + pb->logical_port);
> + if (b_lport) {
> + /* If the 'b_lport->type' and 'lport_type' don't match, then update
> + * the b_lport->type to the updated 'lport_type'. The function
> + * binding_lport_check_and_cleanup() will cleanup the 'b_lport'
> + * if required. */
> + if (b_lport->type != lport_type) {
> + b_lport->type = lport_type;
> }
> + b_lport = binding_lport_check_and_cleanup(
> + b_lport, &b_ctx_out->lbinding_data->lports);
> + }
> +
> + if (lport_type == LP_VIF) {
> + 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 +2332,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 +2386,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 +2549,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 +2589,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 +2624,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 +2640,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 +2652,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 +2673,305 @@ 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)) {
> + 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.
> + */
> +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 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;
> +}
> +
> +/* This function checks and cleans up the 'b_lport' if it is
> + * not in the correct state.
> + *
> + * If the 'b_lport' type is LP_VIF, then its name and its lbinding->name
> + * should match. Otherwise this should be cleaned up.
> + *
> + * If the 'b_lport' type is LP_CONTAINER, then its parent_port name should
> + * be the same as its lbinding's name. Otherwise this should be
> + * cleaned up.
> + *
> + * If the 'b_lport' type is LP_VIRTUAL, then its virtual parent name
> + * should be the same as its lbinding's name. Otherwise this
> + * should be cleaned up.
> + *
> + * If the 'b_lport' type is not LP_VIF, LP_CONTAINER or LP_VIRTUAL, it
> + * should be cleaned up. This can happen if the CMS changes
> + * the port binding type.
> + */
> +static struct binding_lport *
> +binding_lport_check_and_cleanup(struct binding_lport *b_lport,
> + struct shash *binding_lports)
> +{
> + 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..4fc9ef207b 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -36,6 +36,7 @@ struct sbrec_chassis;
> struct sbrec_port_binding_table;
> struct sset;
> struct sbrec_port_binding;
> +struct ds;
>
> struct binding_ctx_in {
> struct ovsdb_idl_txn *ovnsb_idl_txn;
> @@ -56,7 +57,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 +87,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 *pb_name);
>
> /* Represents a tracked binding logical port. */
> struct tracked_binding_lport {
> @@ -128,8 +117,6 @@ bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> const struct sbrec_port_binding_table *,
> const struct sbrec_chassis *);
>
> -void local_bindings_init(struct shash *local_bindings);
> -void local_bindings_destroy(struct shash *local_bindings);
> bool binding_handle_ovs_interface_changes(struct binding_ctx_in *,
> struct binding_ctx_out *);
> bool binding_handle_port_binding_changes(struct binding_ctx_in *,
> @@ -137,7 +124,8 @@ 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);
> +void binding_dump_local_bindings(struct local_binding_data *, struct ds *);
> #endif /* controller/binding.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index ea0b677bdc..7ee11dffe2 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -81,6 +81,7 @@ static unixctl_cb_func cluster_state_reset_cmd;
> static unixctl_cb_func debug_pause_execution;
> static unixctl_cb_func debug_resume_execution;
> static unixctl_cb_func debug_status_execution;
> +static unixctl_cb_func debug_dump_local_bindings;
> static unixctl_cb_func lflow_cache_flush_cmd;
> static unixctl_cb_func lflow_cache_show_stats_cmd;
> static unixctl_cb_func debug_delay_nb_cfg_report;
> @@ -1182,8 +1183,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 +1222,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 +1251,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 +1294,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 +1322,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 +1405,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 +1449,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 +1460,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 +1822,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;
> }
>
> @@ -2688,7 +2688,8 @@ main(int argc, char *argv[])
> engine_get_internal_data(&en_flow_output);
> struct ed_type_ct_zones *ct_zones_data =
> engine_get_internal_data(&en_ct_zones);
> - struct ed_type_runtime_data *runtime_data = NULL;
> + struct ed_type_runtime_data *runtime_data =
> + engine_get_internal_data(&en_runtime_data);;
Nit: one ';' too many.
>
> ofctrl_init(&flow_output_data->group_table,
> &flow_output_data->meter_table,
> @@ -2741,6 +2742,11 @@ main(int argc, char *argv[])
> unixctl_command_register("debug/delay-nb-cfg-report", "SECONDS", 1, 1,
> debug_delay_nb_cfg_report, &delay_nb_cfg_report);
>
> + unixctl_command_register("debug/dump-local-bindings", "", 0, 0,
> + debug_dump_local_bindings,
> + &runtime_data->lbinding_data);
> + runtime_data = NULL;
I think it's fine to remove this line, next time we use 'runtime_data'
we reinitialize it anyway.
> +
> unsigned int ovs_cond_seqno = UINT_MAX;
> unsigned int ovnsb_cond_seqno = UINT_MAX;
> unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
> @@ -2958,7 +2964,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);
> @@ -2971,7 +2977,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);
> }
> }
>
> @@ -3411,3 +3417,13 @@ debug_delay_nb_cfg_report(struct unixctl_conn *conn, int argc OVS_UNUSED,
> unixctl_command_reply(conn, "no delay for nb_cfg report.");
> }
> }
> +
> +static void
> +debug_dump_local_bindings(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *local_bindings)
> +{
> + struct ds binding_data = DS_EMPTY_INITIALIZER;
> + binding_dump_local_bindings(local_bindings, &binding_data);
> + unixctl_command_reply(conn, ds_cstr(&binding_data));
> + ds_destroy(&binding_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-controller.at b/tests/ovn-controller.at
> index 02212ed9c5..a6024468e7 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -469,3 +469,83 @@ OVS_WAIT_UNTIL([
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +# Test that changes of a port binding from one type to another doesn'that
> +# result in any ovn-controller asserts or crashes.
> +AT_SETUP([ovn-controller - port binding type change handling])
> +AT_KEYWORDS([ovn])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +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
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> +primary lport : [[lsp1]]
> +----------------------------------------
> +])
> +
> +# pause ovn-northd
> +check as northd ovn-appctl -t ovn-northd pause
> +check as northd-backup ovn-appctl -t ovn-northd pause
> +
> +as northd ovn-appctl -t ovn-northd status
> +as northd-backup ovn-appctl -t ovn-northd status
> +
> +for type in patch chassisredirect l3gateway localnet localport l2gateway \
> +virtual external remote vtep
> +do
> + for update_type in patch chassisredirect l3gateway localnet localport l2gateway \
> +virtual external remote vtep
> + do
I think you missed 'patch' type.
Also, I think this can be simplified to avoid duplication if we use
arrays, for example:
pb_types=(patch chassisredirect l3gateway localnet localport l2gateway
virtual external remote vtep)
for type in ${pb_types[[@]]}
do
for update_type in ${pb_types[[@]]}
do
...
> + check ovn-sbctl set port_binding lsp1 type=$type
> + check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=$type
> + OVS_WAIT_UNTIL([test $type = $(ovn-sbctl get chassis . other_config:ovn-cms-options)])
> +
> + AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> + echo "Updating to $update_type from $type"
> + check ovn-sbctl set port_binding lsp1 type=$update_type
> + check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=$update_type
> + OVS_WAIT_UNTIL([test $update_type = $(ovn-sbctl get chassis . other_config:ovn-cms-options)])
> +
> + AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[0]]
> +----------------------------------------
> +])
> + # Set the port binding type back to VIF.
> + check ovn-sbctl set port_binding lsp1 type=\"\"
> + check as hv1 ovs-vsctl set open . external_ids:ovn-cms-options=foo
> + OVS_WAIT_UNTIL([test foo = $(ovn-sbctl get chassis . other_config:ovn-cms-options)])
> +
> + AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[lsp1]], OVS interface name : [[vif1]], num binding lports : [[1]]
> +primary lport : [[lsp1]]
> +----------------------------------------
> +])
> + done
> +done
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 391a8bcd93..73cc8bf025 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -25216,3 +25216,643 @@ 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
> +
> +# Delete vm1, vm-cont1 and vm-cont2 and recreate again.
> +check ovn-nbctl lsp-del vm1
> +check ovn-nbctl lsp-del vm-cont1
> +check ovn-nbctl --wait=hv lsp-del vm-cont2
> +
> +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm1
> +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
> +
> +# Make vm1 as a child port of some non existent lport - foo. vm1, vm1-cont1 and
> +# vm1-cont2 should be released.
> +check ovn-nbctl --wait=sb set logical_switch_port vm1 parent_name=bar
> +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
> +
> +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
> +])
> +
> +# Tests that ovn-controller creates local bindings correctly by running
> +# ovn-appctl -t ovn-controller debug/dump-local-bindings.
> +# Ideally this test case should have been a unit test case.
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ovn-controller local bindings])
> +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 hv1-vm1
> +
> +sim_add hv2
> +as hv2
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.2
> +ovs-vsctl -- add-port br-int hv2-vm1
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lsp-add sw0 sw0p1
> +check ovn-nbctl lsp-add sw0 sw0p2
> +
> +check as hv1 ovs-vsctl set interface hv1-vm1 external_ids:iface-id=sw0p1
> +check as hv2 ovs-vsctl set interface hv2-vm1 external_ids:iface-id=sw0p2
> +
> +wait_for_ports_up
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv1-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p1]]
> +----------------------------------------
> +])
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +])
> +
> +# Create an ovs interface in hv1
> +check as hv1 ovs-vsctl add-port br-int hv1-vm2 -- set interface hv1-vm2 external_ids:iface-id=sw1p1
> +check ovn-nbctl --wait=hv sync
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv1-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p1]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# Create lport sw1p1
> +check ovn-nbctl ls-add sw1 -- lsp-add sw1 sw1p1
> +
> +wait_for_ports_up
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv1-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p1]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[1]]
> +primary lport : [[sw1p1]]
> +----------------------------------------
> +])
> +
> +# Swap sw0p1 and sw0p2.
> +check as hv1 ovs-vsctl set interface hv1-vm1 external_ids:iface-id=sw0p2
> +check as hv2 ovs-vsctl set interface hv2-vm1 external_ids:iface-id=sw0p1
> +
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p2]], OVS interface name : [[hv1-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[1]]
> +primary lport : [[sw1p1]]
> +----------------------------------------
> +])
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv2-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p1]]
> +----------------------------------------
> +])
> +
> +# Create child port for sw0p1
> +check ovn-nbctl --wait=hv lsp-add sw0 sw0p1-c1 sw0p1 1
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +primary lport : [[sw0p1]]
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv1-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[1]]
> +primary lport : [[sw1p1]]
> +----------------------------------------
> +])
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv2-vm1]], num binding lports : [[2]]
> +primary lport : [[sw0p1]]
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +----------------------------------------
> +])
> +
> +# Create another child port for sw0p1
> +check ovn-nbctl --wait=hv lsp-add sw0 sw0p1-c2 sw0p1 2
> +
> +wait_for_ports_up
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[3]]
> +primary lport : [[sw0p1]]
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv1-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[1]]
> +primary lport : [[sw1p1]]
> +----------------------------------------
> +])
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv2-vm1]], num binding lports : [[3]]
> +primary lport : [[sw0p1]]
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +])
> +
> +# Swap sw0p1 and sw0p2 again.
> +check as hv1 ovs-vsctl set interface hv1-vm1 external_ids:iface-id=sw0p1
> +check as hv2 ovs-vsctl set interface hv2-vm1 external_ids:iface-id=sw0p2
> +
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[hv1-vm1]], num binding lports : [[3]]
> +primary lport : [[sw0p1]]
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[1]]
> +primary lport : [[sw1p1]]
> +----------------------------------------
> +])
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[3]]
> +primary lport : [[sw0p1]]
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +])
> +
> +# Make sw0p1 as child port of non existent lport - foo
> +check ovn-nbctl --wait=hv set logical_switch_port sw0p1 parent_name=foo
> +
> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[hv1-vm1]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw1p1]], OVS interface name : [[hv1-vm2]], num binding lports : [[1]]
> +primary lport : [[sw1p1]]
> +----------------------------------------
> +])
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +])
> +
> +# Change the lport type of sw0p2 to different types and make sure that
> +# local bindings are correct.
> +
> +hv2_uuid=$(fetch_column Chassis _uuid name=hv2)
> +check_column "$hv2_uuid" Port_Binding chassis logical_port=sw0p2
> +
> +# Change the port type to router, ovn-controller should release it.
> +check ovn-nbctl --wait=hv lsp-set-type sw0p2 router
> +check_column "" Port_Binding chassis logical_port=sw0p2
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# change the port type to external from router.
> +check ovn-nbctl --wait=hv lsp-set-type sw0p2 external
> +check_column "" Port_Binding chassis logical_port=sw0p2
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# change the port type to localnet from external.
> +check ovn-nbctl --wait=hv lsp-set-type sw0p2 localnet
> +check_column "" Port_Binding chassis logical_port=sw0p2
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# change the port type to localport from localnet.
> +check ovn-nbctl --wait=hv lsp-set-type sw0p2 localnet
> +check_column "" Port_Binding chassis logical_port=sw0p2
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[0]]
> +----------------------------------------
> +])
> +
> +# change the port type back to vif.
> +check ovn-nbctl --wait=hv lsp-set-type sw0p2 ""
> +wait_column "$hv2_uuid" Port_Binding chassis logical_port=sw0p2
> +
> +AT_CHECK([as hv2 ovn-appctl -t ovn-controller debug/dump-local-bindings], [0], [dnl
> +Local bindings:
> +name: [[foo]], OVS interface name : [[NULL]], num binding lports : [[1]]
> +no primary lport
> +child lport[[1]] : [[sw0p1]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p1]], OVS interface name : [[NULL]], num binding lports : [[2]]
> +no primary lport
> +child lport[[1]] : [[sw0p1-c1]], type : [[CONTAINER]]
> +child lport[[2]] : [[sw0p1-c2]], type : [[CONTAINER]]
> +----------------------------------------
> +name: [[sw0p2]], OVS interface name : [[hv2-vm1]], num binding lports : [[1]]
> +primary lport : [[sw0p2]]
> +----------------------------------------
> +])
> +
> +OVN_CLEANUP([hv1], [hv2])
> +AT_CLEANUP
> +])
>
More information about the dev
mailing list