On Thu, Aug 14, 2025 at 8:55 AM Ales Musil <[email protected]> wrote:
> > > On Tue, Aug 12, 2025 at 8:22 PM Ilya Maximets <[email protected]> wrote: > >> On 8/11/25 11:13 PM, Rosemarie O'Riorden wrote: >> > Previously, if there was a logical switch with a virtual port that had a >> > virtual parent port configured, and then the parent port was deleted and >> > recreated, the parent port would not be reclaimed by the chassis and >> > thus would not behave as the parent port, breaking traffic. >> > >> > This is because virtual ports were not being updated if their parents >> > were recently created or deleted. >> > >> > So long as virtual ports have a parent port configured, there should be >> > no issue deleting and recreating the parent. It should continue working >> > as the parent as soon as it's up again. >> > >> > With this change, the parent port will function as the parent even after >> > deletion and recreation. The chassis will properly reclaim the virtual >> > port. >> > >> > Fixes: b337750e45be ("northd: Incremental processing of VIF changes in >> 'northd' node.") >> > Reported-at: https://issues.redhat.com/browse/FDP-710 >> > Co-authored-by: Mohammad Heib <[email protected]> >> > Signed-off-by: Mohammad Heib <[email protected]> >> > Signed-off-by: Rosemarie O'Riorden <[email protected]> >> > --- >> > v4: >> > - Added Fixes tag. >> > - Combined two ssets for deleted and created ports into one. >> > - Nest looping through list under condition that sset is not empty. >> > - Only destory sset if not empty, otherwise unnecessary. >> > - Spacing, ordering, and other syntax nits. >> > >> > v3: >> > - Fixed algorithm to use an sset and loop through created ports, only >> > updating them if their virtual parent was recently created or >> > deleted. >> > - Added case for deleted parents, as just mentioned. >> > - Fixed send_garp() syntax in test and remove redefinition. >> > - Added wait for port binding to disappear in test. >> > - Added more virtual parents to test. >> > - Fixed various syntax issues/nits. >> > >> > v2: Fixed memory leak by freeing op_smap and tokstr. >> > --- >> > northd/northd.c | 45 +++++++++++++++++++++++++ >> > tests/ovn.at | 89 +++++++++++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 134 insertions(+) >> > >> > diff --git a/northd/northd.c b/northd/northd.c >> > index 2cb69f9aa..742d39a72 100644 >> > --- a/northd/northd.c >> > +++ b/northd/northd.c >> > @@ -4559,7 +4559,10 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> > bool ls_had_only_router_ports = >> (!vector_is_empty(&od->router_ports) >> > && (vector_len(&od->router_ports) == >> hmap_count(&od->ports))); >> > >> > + struct ovs_list existing_virtual_ports; >> > struct ovn_port *op; >> > + >> > + ovs_list_init(&existing_virtual_ports); >> > HMAP_FOR_EACH (op, dp_node, &od->ports) { >> > op->visited = false; >> > } >> > @@ -4629,6 +4632,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> > delete_fdb_entries(ni->sbrec_fdb_by_dp_and_port, >> > od->tunnel_key, old_tunnel_key); >> > } >> > + } else if (!strcmp(op->nbsp->type, "virtual")) { >> > + ovs_list_push_back(&existing_virtual_ports, &op->list); >> > } >> > op->visited = true; >> > } >> > @@ -4675,6 +4680,46 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> > } >> > } >> > >> > + /* Update old virtual ports that have newly created or newly >> deleted >> > + * VIF as parent port. This code handles cases where the virtual >> port was >> > + * created before the parent port or when the parent port was >> recreated. >> > + */ >> > + struct hmapx_node *hmapx_node; >> > + >> > + struct sset created_or_deleted_ports; >> > + sset_init(&created_or_deleted_ports); >> > + >> > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) { >> > + op = hmapx_node->data; >> > + sset_add(&created_or_deleted_ports, op->nbsp->name); >> > + } >> > + >> > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->deleted) { >> > + op = hmapx_node->data; >> > + sset_add(&created_or_deleted_ports, op->nbsp->name); >> > + } >> > + >> > + if (!sset_is_empty(&created_or_deleted_ports)) { >> > > + LIST_FOR_EACH_POP (op, list, &existing_virtual_ports) { >> > + const char *virtual_parents = >> > + smap_get_def(&op->nbsp->options, >> > + "virtual-parents", ""); >> >> nit: I'd move the line to the left a bit and just split in 2 instead of 3. >> >> > + char *tokstr = xstrdup(virtual_parents); >> > + char *save_ptr = NULL; >> > + char *vparent; >> > + >> > + for (vparent = strtok_r(tokstr, ",", &save_ptr); vparent >> != NULL; >> > + vparent = strtok_r(NULL, ",", &save_ptr)) { >> > + if (sset_find(&created_or_deleted_ports, vparent)) { >> > + add_op_to_northd_tracked_ports(&trk_lsps->updated, >> op); >> > + break; >> > + } >> > + } >> > + free(tokstr); >> > + } >> > + sset_destroy(&created_or_deleted_ports); >> >> While technically current implementation of sset doesn't allocate any >> memory >> for the empty set, it's not guaranteed, so we should still call the >> destroy() >> after we called init(). So, this line move this out of the if statement. >> >> Other than that the patch looks good to me. Thanks! >> >> Maybe maintainers can apply the following diff while picking up the fix: >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 742d39a72..e380e0bd3 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4702,8 +4702,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> if (!sset_is_empty(&created_or_deleted_ports)) { >> LIST_FOR_EACH_POP (op, list, &existing_virtual_ports) { >> const char *virtual_parents = >> - smap_get_def(&op->nbsp->options, >> - "virtual-parents", ""); >> + smap_get_def(&op->nbsp->options, "virtual-parents", ""); >> char *tokstr = xstrdup(virtual_parents); >> char *save_ptr = NULL; >> char *vparent; >> @@ -4717,8 +4716,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> } >> free(tokstr); >> } >> - sset_destroy(&created_or_deleted_ports); >> } >> + sset_destroy(&created_or_deleted_ports); >> >> return true; >> >> --- >> >> With that, >> >> Acked-by: Ilya Maximets <[email protected]> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Thank you Rosemarie, Mohammad and Ilya, > > I have fixed both comments and applied it to the main. > > Regards, > Ales > Hi Rosemarie and Ilya, I have backported this patch all the way down to 24.03 based on our offline conversation. Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
