On Tue, May 10, 2022 at 10:55 AM Mark Michelson <[email protected]> wrote:
> Thanks Mohammad, > > Acked-by: Mark Michelson <[email protected]> > Thanks. I applied this patch to the main branch and backported to branch-22.03 and branch-21.12. Numan > > On 4/26/22 05:49, Mohammad Heib wrote: > > 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 | 94 +++++++++++++++++++++++++++++++++++++++++-------- > > tests/ovn.at | 12 +++++++ > > 2 files changed, 92 insertions(+), 14 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 8c4187eae..fceba0b2a 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -1814,6 +1814,38 @@ 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) > > +{ > > + *is_old_container_lport = false; > > + if (!sb || !nbsp) { > > + return false; > > + } > > + > > + if (!sb->type[0] && !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)) { > > + *is_old_container_lport = true; > > + return true; > > + } else { > > + return false; > > + } > > + } > > + > > + /* Both lports are not "VIF's" it is safe to use strcmp. */ > > + if (sb->type[0] && nbsp->type[0]) { > > + return strcmp(sb->type, nbsp->type); > > + } > > + > > + return true; > > +} > > + > > static bool > > lrport_is_enabled(const struct nbrec_logical_router_port *lrport) > > { > > @@ -2498,22 +2530,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 sbrec of the affected > lport from > > + * the port_binding table. > > + * > > + * 2. create a new sbrec 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 34b6abfc0..84ddf405e 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -28547,6 +28547,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 > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
