On Thu, Dec 17, 2020 at 10:33 PM Han Zhou <[email protected]> wrote: > > 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.
I had started working on the split of physical flow and logical flow output [1], but I got busy with other stuff and didn't resume this activity, which I should have (although I never forgot about it :)). It's time to revisit that I suppose. I will work on it. [1] - https://github.com/numansiddique/ovn/commit/085879980c597f5fff71c7a799c80f5594e62e32 Thanks Numan > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
