On Thu, Dec 17, 2020 at 6:28 AM Mark Michelson <[email protected]> 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.
Hi Mark, your concern is reasonable. It seems we are now paying the debt for not following the I-P model strictly. The en_physical_flow_changes_handler node is a wrapper node added for some convenient outcome, but it also added some tricky maintainability issues. It seems this fix may also further deviate on this path, and it becomes harder and harder to maintain the correctness of the engine going down this path. As mentioned by Numans, the dependency graph should be fixed by separating the flow_output to physical flows and logical flows, as we had discussed during the reviews when the en_physical_flow_changes_handler was initially added, to strictly follow the dependency graph and declarative data processing and avoid relying on special "triggers" in the code or ordering of change handlers. This was a big TODO. Please find the context here: https://mail.openvswitch.org/pipermail/ovs-dev/2020-May/370940.html Nevertheless, I agree to quickly fix it now with the approach used by this patch. Thanks, Han > > That being said, I pushed this to master and branch-20.12. > > >> Thanks > >> Numan > >> > > > > Regards, > > Dumitru > > > > _______________________________________________ > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
