[ovs-dev] [PATCH] ofp-monitor: Extend Flow Monitoring support for OF 1.0+ with Nicira Extensions

[Vasu Dasari]

Thanks Ben for your comments.

1. Adding Nicira extensions for Flow Monitoring was pretty straightforward
with the existing code base. As you might have seen, I just had to tweak
some functions to get this to work. I do not have any hard opinions against
your point that we should not duplicate functionality with Nicira
extensions and OpenFlow 1.4,1.5. But my only suggestion is to make the
support for OF 1.4, 1.5 flow monitoring messages to be implemented as a
separate commit, as I think it is going to add new functions to make that
happen.

2. Sure, I will add this support announcement to NEWS.

3. I saw the test cases you identified and I like your idea of writing a
wrapper function to test those test cases across all possible versions. My
take is, if one test case is written thoroughly to cover all versions with
some basic test, that should suffice testing core functionality as it is
the same across all versions. That is why I have extended "ofproto - flow
monitoring usable protocols" to test other protocols. I will look into your
suggestion.

Thanks
-Vasu

*Vasu Dasari*


On Mon, May 10, 2021 at 3:27 PM Ben Pfaff <blp at ovn.org> wrote:

> On Sat, May 08, 2021 at 07:12:27AM -0400, Vasu Dasari wrote:
> > Currently OVS supports flow-monitoring for OpenFlow 1.0 and Nicira
> Extenstions.
> > Any other OpenFlow versioned messages are not accepted. This checkin
> will allow
> > OpenFlow1.0+ Flow Monitoring wth Nicira extensions be accepted. Also
> made sure
> > that flow-monitoring updates, flow monitoring pause messages, resume
> messages
> > are sent in the same OpenFlow version as that of flow-monitor request.
> >
> > Description of changes:
> >
> > 1. Generate ofp-msgs.inc to be able to support 1.0+ Flow Monitoring
> messages.
> > include/openvswitch/ofp-msgs.h
> >
> > 2. Support vconn to accept user specified version and use it for vconn
> flow-monitoring session
> > ofproto/ofproto.c
> >
> > 3. Modify APIs to use protocol as an argument to encode and decode
> messages
> > include/openvswitch/ofp-monitor.h
> > include/openvswitch/vlog.h
> > lib/ofp-monitor.c
> > ofproto/connmgr.c
> > ofproto/connmgr.h
> > ofproto/ofproto.c
> >
> >  4. Testcase for verification
> > tests/ofproto.at
> >
> > Signed-off-by: Vasu Dasari <vdasari at gmail.com>
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050820.html
> > Signed-off-by: Vasu Dasari <vdasari at gmail.com>
>
> Thanks for working on this.
>
> OpenFlow 1.1-1.3 don't have flow monitoring support built into the
> protocol, so it makes sense to support it as an extension.  This commit
> adds the entension for 1.4 and 1.5 as well.  I am not convinced that
> makes sense, since OF1.4+ has its own support.  It's not great to
> duplicate functionality if we can avoid it.
>
> This should add an item in NEWS.  I think an update to documentation for
> ovs-ofctl might be warranted too, although I didn't look.
>
> The update to the tests here just check that the extension appears
> available.  There are multiple other flow monitoring tests:
>
>    4579:AT_SETUP([ofproto - flow monitoring])
>    4716:AT_SETUP([ofproto - flow monitoring with !own])
>    4757:AT_SETUP([ofproto - flow monitoring with out_port])
>    4809:AT_SETUP([ofproto - flow monitoring pause and resume])
>
> It would be good to run at least some of these tests with those other
> versions as well.  They are fairly big, so cutting and pasting wouldn't
> be ideal; perhaps some m4 macro work or shell function work would be
> better.
>
> Thanks,
>
> Ben.
>