On Fri, Dec 18, 2020 at 1:22 AM Numan Siddique <[email protected]> wrote: > > 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 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
