| Message ID | 20220426094947.912610-1-mheib@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v3] northd: handle container lport type update | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Thanks Mohammad, Acked-by: Mark Michelson <mmichels@redhat.com> 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 <mheib@redhat.com> > --- > 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 >
On Tue, May 10, 2022 at 10:55 AM Mark Michelson <mmichels@redhat.com> wrote: > Thanks Mohammad, > > Acked-by: Mark Michelson <mmichels@redhat.com> > 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 <mheib@redhat.com> > > --- > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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
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 <mheib@redhat.com> --- northd/northd.c | 94 +++++++++++++++++++++++++++++++++++++++++-------- tests/ovn.at | 12 +++++++ 2 files changed, 92 insertions(+), 14 deletions(-)