On 12/17/20 3:28 PM, Mark Michelson wrote: > On 12/17/20 8:56 AM, Dumitru Ceara wrote: >> On 12/17/20 2:30 PM, Numan Siddique wrote: >>> On Thu, Dec 17, 2020 at 3:47 PM Dumitru Ceara <[email protected]> wrote: >>>> >>>> On 12/17/20 10:23 AM, Dumitru Ceara wrote: >>>>> The incremental processing engine implementation is such that if an >>>>> input node gets updated but the output node doesn't have a change >>>>> handler for it then the output node is immediately recomputed. >>>>> That is, >>>>> no other input change handlers are executed. >>>>> >>>>> In case of the en_physical_flow_changes node, if a ct-zone changes we >>>>> were also skipping the OVS interface change handler. That is >>>>> incorrect >>>>> as there is an implicit requirement that flows for OVS interfaces that >>>>> got deleted should be cleared before physical_run() is called. >>>>> >>>>> Instead, we now add an explicit change handler for ct-zone changes. >>>>> This change handler never processes ct-zone updates incrementally but >>>>> ensures that the OVS interface change handler is also called. >>>>> >>>>> Moreover, OVS interface changes (including deletes) are now processed >>>>> before physical_run() is called in the flow_output >>>>> physical_flow_changes_handler. This is a requirement because >>>>> otherwise >>>>> physical_run() will fail to add flows for new OVS interfaces that >>>>> correspond to unchanged Port_Bindings. >>>>> >>>>> Reported-by: Daniel Alvarez <[email protected]> >>>> >>>> Reported-by: Krzysztof Klimonda <[email protected]> >>>> >>>> Sorry, I forgot to credit Chris for the report too. I can add the >>>> above >>>> in a v2 if needed but I'll wait for some reviews before that. >>> >>> Thanks for the fix. >>> >>> Acked-by: Numan Siddique <[email protected]> >>> >> >> Thanks for the review! >> >>> I think if we were maintaining a separate flow table for physical >>> flows, we could >>> have cleared that flow table before calling physical_run. >>> >> >> This might work too indeed. Also, I think the incremental processing >> engine could also be improved to avoid the ambiguity between NULL change >> handler and a change handler that always return false. >> >> In either case, I think it's better to make such intrusive changes only >> after we release 20.12. >> > > I agree on waiting until after 20.12 is released. I think this change is > indicative of other underlying issues, too. It's odd that skipping a > change handler results in some computation not happening during the > recompute. > > That being said, I pushed this to master and branch-20.12. >
Thanks Mark! I sent backport patches for branch-20.09 and branch-20.06 (only with minor changes in the test): http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
