[ovs-dev] [PATCH ovn] ovn-controller: Always run the I-P OVS Interface change handler.

[Han Zhou]

On Fri, Dec 18, 2020 at 1:22 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Thu, Dec 17, 2020 at 10:33 PM Han Zhou <zhouhan at gmail.com> wrote:
> >
> > On Thu, Dec 17, 2020 at 6:28 AM Mark Michelson <mmichels at redhat.com>
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 <dceara at redhat.com>
> > 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 <dalvarez at redhat.com>
> > > >>>
> > > >>> Reported-by: Krzysztof Klimonda <kklimonda at syntaxhighlighted.com>
> > > >>>
> > > >>> 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 <numans at ovn.org>
> > > >>
> > > >
> > > > 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
> > > > dev at openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >