On 2/28/25 9:01 AM, Ales Musil wrote: > On Thu, Feb 13, 2025 at 3:52 PM Max Lamprecht via dev < > [email protected]> wrote: > >> We don't need to do a full lflow recompute for non_vif_data changes. >> non_vif_data is only used to retrieve the local openflow port of an >> OVN port that is currently bound to the chassis and this will never change. >> >> Signed-off-by: Max Lamprecht <[email protected]> >> --- >> > > Hi Max,
Hi Max, Ales, > thank you for the patch. However I don't agree with the conclusion. > The ofport can actually change even after the port and interface are > created. So this approach would mean that lflow referencing this > ofport number would be wrong until the next recompute of the lflow. It is It seems you're right, Ales. There's no guarantee the next recompute of the lflow will happen anytime soon so the time we maintain incorrect openflows might be relatively long. It seems risky. > extremely rare that this would happen, maybe we could have handler > which would check the ofport changes. Also what change in the non_vif > triggers the recompute? > It might not be that straightforward to add an en_lflow change handler for en_non_vif_data changes but what might be easier (and I think you're alluding towards that Ales) is to modify the local_nonvif_data_handle_ovs_iface_changes() handler to not trigger recomputes on tunnel and patch interface irrelevant changes. We could say we only trigger recomputes in that change handler if the br-int tunnel ports ovn external-ids change or if the patch ports ofport changes. > CCing maintainers maybe the "risk" of having this bug is acceptable. > > >> controller/ovn-controller.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 194d2e2fe..d76067e30 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -5354,8 +5354,10 @@ main(int argc, char *argv[]) >> lflow_output_template_vars_handler); >> engine_add_input(&en_lflow_output, &en_runtime_data, >> lflow_output_runtime_data_handler); >> + /* Using a noop handler because we only need the non_vif_data to find >> + * the corresponding local openflow port of an OVN port. */ >> engine_add_input(&en_lflow_output, &en_non_vif_data, >> - NULL); >> + engine_noop_handler); The en_pflow_output node still does a full recompute on non_vif_data changes FWIW. >> >> engine_add_input(&en_lflow_output, &en_sb_multicast_group, >> lflow_output_sb_multicast_group_handler); >> -- >> 2.34.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Thanks, > Ales > Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
