currently, when a lport with a nonempty parent_name field is created or updated in the NBDB the ovn-controller will handle this port as a container lport and will do all the required operations to maintain this lport.
This behavior will be changed if the user has explicitly removed the parent_name field from the NBDB and the ovn-controller will start handler this port as a VIF port without cleaning its previous allocations and without removing its previous relations with other lports and that can lead to undefined behavior when handling such types of lports. This patch trying to fix the above issue by removing the port_binding row for a container port that its parent_port got removed and that makes the ovn-controller remove all its allocations and relations with other ports and then create a new port_binding row with the same logical_port as the deleted row and let northd handle it as a newly created port. This approach can be applied to other lport types change not only for container port but for now it's only handling container lport type change. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2037433 Signed-off-by: Mohammad Heib <[email protected]> --- northd/northd.c | 92 +++++++++++++++++++++++++++++++++++++++++-------- tests/ovn.at | 12 +++++++ 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 2fb0a93c2..1a06ca345 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -1801,6 +1801,36 @@ lsp_is_remote(const struct nbrec_logical_switch_port *nbsp) return !strcmp(nbsp->type, "remote"); } +static bool +lsp_is_type_changed(const struct sbrec_port_binding *sb, + const struct nbrec_logical_switch_port *nbsp, + bool *is_old_container_lport) +{ + if (!sb || !nbsp) { + return false; + } + /* Both lports are not "VIF's" it is safe to use strcmp. */ + if ((sb->type && sb->type[0]) && + (nbsp->type && nbsp->type[0])) { + return strcmp(sb->type, nbsp->type); + } else if ((!sb->type || !sb->type[0]) && + (!nbsp->type || !nbsp->type[0])) { + /* Two "VIF's" interface make sure both have parent_port + * set or both have parent_port unset, otherwisre they are + * different ports type. + */ + if ((sb->parent_port && nbsp->parent_name) || + (!sb->parent_port && !nbsp->parent_name)) { + return false; + } else { + *is_old_container_lport = true; + return true; + } + } + + return true; +} + static bool lrport_is_enabled(const struct nbrec_logical_router_port *lrport) { @@ -2481,22 +2511,56 @@ join_logical_ports(struct northd_input *input_data, VLOG_WARN_RL(&rl, "duplicate logical port %s", nbsp->name); continue; } else if (op && (!op->sb || op->sb->datapath == od->sb)) { - ovn_port_set_nb(op, nbsp, NULL); - ovs_list_remove(&op->list); - - uint32_t queue_id = smap_get_int(&op->sb->options, - "qdisc_queue_id", 0); - if (queue_id && op->sb->chassis) { - add_chassis_queue( - chassis_qdisc_queues, &op->sb->chassis->header_.uuid, - queue_id); - } + /* + * Handle cases where lport type was explicitly changed + * in the NBDB, in such cases: + * 1. remove the current sberc of the affected lport from + * the port_binding table. + * + * 2. create a new sberc with the same logical_port as the + * deleted lport and add it to the nb_only list which + * will make the northd handle this lport as a new + * created one and recompute everything that is needed + * for this lport. + * + * This change will affect container lport type changes + * only for now, this change is needed in container + * lport cases to avoid port type conflicts in the + * ovn-controller when the user clears the parent_port + * field in the container lport. + * + * This approach can be applied to all other lport types + * changes by removing the is_old_container_lport. + */ + bool is_old_container_lport = false; + if (op->sb && lsp_is_type_changed(op->sb, nbsp, + &is_old_container_lport) + && is_old_container_lport) { + ovs_list_remove(&op->list); + sbrec_port_binding_delete(op->sb); + ovn_port_destroy(ports, op); + op = ovn_port_create(ports, nbsp->name, nbsp, + NULL, NULL); + ovs_list_push_back(nb_only, &op->list); + } else { + ovn_port_set_nb(op, nbsp, NULL); + ovs_list_remove(&op->list); + + uint32_t queue_id = smap_get_int(&op->sb->options, + "qdisc_queue_id", 0); + if (queue_id && op->sb->chassis) { + add_chassis_queue( + chassis_qdisc_queues, + &op->sb->chassis->header_.uuid, + queue_id); + } - ovs_list_push_back(both, &op->list); + ovs_list_push_back(both, &op->list); - /* This port exists due to a SB binding, but should - * not have been initialized fully. */ - ovs_assert(!op->n_lsp_addrs && !op->n_ps_addrs); + /* This port exists due to a SB binding, but should + * not have been initialized fully. */ + ovs_assert(!op->n_lsp_addrs && !op->n_ps_addrs); + } } else { op = ovn_port_create(ports, nbsp->name, nbsp, NULL, NULL); ovs_list_push_back(nb_only, &op->list); diff --git a/tests/ovn.at b/tests/ovn.at index 0c2fe9f97..0ff0a67ab 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -28244,6 +28244,18 @@ check ovn-nbctl lsp-add ls vm-cont2 vm1 2 wait_for_ports_up +check ovn-nbctl clear logical_switch_port vm-cont1 parent_name +check ovn-nbctl clear logical_switch_port vm-cont2 parent_name + +wait_column "true" sb:Port_Binding up logical_port=vm1 +wait_column "false" sb:Port_Binding up logical_port=vm-cont1 +wait_column "false" sb:Port_Binding up logical_port=vm-cont2 + +check ovn-nbctl set logical_switch_port vm-cont1 parent_name=vm1 +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm1 + +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 -- 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
