Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
On Tue, Feb 5, 2019 at 4:41 PM Ben Pfaff wrote: > On Tue, Feb 05, 2019 at 04:26:56PM -0800, Ashish Varma wrote: > > In the parse_flow_monitor_request(), usable_protocols is an out > parameter. > > (and is set in parse_flow_monitor_request() only if a match field is > > provided. ) > > allowed_versions on the other hand is used only in the ofctl_monitor > > function. That is the reason for the check in ofctl_monitor. > > The usual way we handle this sort of version dependency is a bitmap like > 'usable_protocols'. It works well for flows, for example. You can see > lots of examples in ovs-ofctl.c if you search for the identifier > usable_protocols. > > Looking closer at this instance, the existing code is buggy. Currently, > only OpenFlow 1.0 should be supported, because that's the only protocol > where OVS actually supports flow monitoring. > parse_flow_monitor_request() should be returning that consistently as > the out parameter (as you noted). Or it should be returning 0 if it's > impossible to request a flow monitor at all (in the case where OF1.0 > can't support whatever field is specified when > parse_flow_monitor_request__ parses a field). But it's buggy and > nothing ever properly initializes it. That bug is masked by another > bug: nothing ever checks it, either! And, finally, there is a third > bug: ovs-ofctl calls ofputil_append_flow_monitor_request() and blindly > always uses OF1.0, which might not be the protocol actually in use on > the vconn. > > The manpage for the ovs-ofctl monitor command doesn't say that "watch:" > is OF1.0 only or that it is an Open vSwitch extension, either, although > it should. > > It would be for the best if we can fix all these bugs before we add > support for OF1.4+ flow monitor, and then backport the bug fixes. Does > my description of the problems make sense? Can you tackle these > problems too? > > Sure. I will try and fix this before the support for OF1.4+ flow monitor. Thanks for explaining the issue. > Thanks, > > Ben. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
On Tue, Feb 05, 2019 at 04:26:56PM -0800, Ashish Varma wrote: > In the parse_flow_monitor_request(), usable_protocols is an out parameter. > (and is set in parse_flow_monitor_request() only if a match field is > provided. ) > allowed_versions on the other hand is used only in the ofctl_monitor > function. That is the reason for the check in ofctl_monitor. The usual way we handle this sort of version dependency is a bitmap like 'usable_protocols'. It works well for flows, for example. You can see lots of examples in ovs-ofctl.c if you search for the identifier usable_protocols. Looking closer at this instance, the existing code is buggy. Currently, only OpenFlow 1.0 should be supported, because that's the only protocol where OVS actually supports flow monitoring. parse_flow_monitor_request() should be returning that consistently as the out parameter (as you noted). Or it should be returning 0 if it's impossible to request a flow monitor at all (in the case where OF1.0 can't support whatever field is specified when parse_flow_monitor_request__ parses a field). But it's buggy and nothing ever properly initializes it. That bug is masked by another bug: nothing ever checks it, either! And, finally, there is a third bug: ovs-ofctl calls ofputil_append_flow_monitor_request() and blindly always uses OF1.0, which might not be the protocol actually in use on the vconn. The manpage for the ovs-ofctl monitor command doesn't say that "watch:" is OF1.0 only or that it is an Open vSwitch extension, either, although it should. It would be for the best if we can fix all these bugs before we add support for OF1.4+ flow monitor, and then backport the bug fixes. Does my description of the problems make sense? Can you tackle these problems too? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
Thanks for the review! In the parse_flow_monitor_request(), usable_protocols is an out parameter. (and is set in parse_flow_monitor_request() only if a match field is provided. ) allowed_versions on the other hand is used only in the ofctl_monitor function. That is the reason for the check in ofctl_monitor. Would add the examples in tests/ofp-print.at I will add the monitoring support for OpenFlow 1.1, 1.2 and 1.3 as a separate patch. Thanks, Ashish On Mon, Feb 4, 2019 at 3:48 PM Ben Pfaff wrote: > On Thu, Jan 31, 2019 at 03:49:39PM -0800, Ashish Varma wrote: > > OVS supports Nicira version of Flow Monitor feature which allows an > OpenFlow > > controller to keep track of any changes in the flow table. (The changes > can > > done by the controller itself or by any other controller connected to > OVS.) > > This patch adds support for the OpenFlow 1.4+ OFPMP_FLOW_MONITOR > multipart > > message. > > Also added support in "ovs-ofctl monitor" to send OpenFlow 1.4+ messages > to > > OVS. > > Added unit test cases to test the OpenFlow version of Flow Monitor > messages. > > > > Signed-off-by: Ashish Varma > > Thanks for the revision! It will be nice to finally have this feature! > > Since parse_flow_monitor_request() already has a 'usable_protocols' > parameter, I would suggest using that to limit the support for > out_group, rather than putting a special case into ofctl_monitor(). > > I would add an example of the new message to tests/ofp-print.at. > > I think we talked about supporting flow monitors in OF1.1, 1.2, and > 1.3. I seem to recall that you plan to add that afterward, in a > separate patch. Is that correct? I hope that, if so, you can start > work on it soon after this patch is accepted. > > I would add an item to NEWS. > > Thanks, > > Ben. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
On Thu, Jan 31, 2019 at 03:49:39PM -0800, Ashish Varma wrote: > OVS supports Nicira version of Flow Monitor feature which allows an OpenFlow > controller to keep track of any changes in the flow table. (The changes can > done by the controller itself or by any other controller connected to OVS.) > This patch adds support for the OpenFlow 1.4+ OFPMP_FLOW_MONITOR multipart > message. > Also added support in "ovs-ofctl monitor" to send OpenFlow 1.4+ messages to > OVS. > Added unit test cases to test the OpenFlow version of Flow Monitor messages. > > Signed-off-by: Ashish Varma Thanks for the revision! It will be nice to finally have this feature! Since parse_flow_monitor_request() already has a 'usable_protocols' parameter, I would suggest using that to limit the support for out_group, rather than putting a special case into ofctl_monitor(). I would add an example of the new message to tests/ofp-print.at. I think we talked about supporting flow monitors in OF1.1, 1.2, and 1.3. I seem to recall that you plan to add that afterward, in a separate patch. Is that correct? I hope that, if so, you can start work on it soon after this patch is accepted. I would add an item to NEWS. Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] ofp-monitor: Added support for OpenFlow 1.4+ Flow Monitor
OVS supports Nicira version of Flow Monitor feature which allows an OpenFlow
controller to keep track of any changes in the flow table. (The changes can
done by the controller itself or by any other controller connected to OVS.)
This patch adds support for the OpenFlow 1.4+ OFPMP_FLOW_MONITOR multipart
message.
Also added support in "ovs-ofctl monitor" to send OpenFlow 1.4+ messages to
OVS.
Added unit test cases to test the OpenFlow version of Flow Monitor messages.
Signed-off-by: Ashish Varma
---
v1-v2:
Aligned the comments on members of structs and enumerations to
a common column.
Added "14" infix for enumeration members of OpenFlow 1.4.
Removed the union of an ofp_port_t and an ofp11_port_t as out_port in
"ofputil_flow_monitor_request".
Removed "ofputil_flow_monitor_flags" enum and use "ofp14_flow_monitor_flags"
flags directly which is a superset of "nx_flow_monitor_flags".
Changed comments to start with a capital letter and end with periods.
Renamed "ofputil_get_util_flow_update_event_from_nx_event()" function to
"ofputil_get_flow_update_event_from_nx_event".
Declared and initialized variables at their first use.
Used OFPUTIL_P_OF10_ANY rather than OFPUTIL_P_OF10_STD_ANY or any specific
OFPUTIL_P_OF10_* protocol.
Fixed parsing of the flags in the "delete" command of the Flow Monitor
request.
Fixed the validation of "out_group" in ovs-ofctl monitor command.
In function "handle_flow_monitor_request", changed "b->data" to "b->head" in
"ofconn_send_error(ofconn, b->data, error)" as ofpbuf b would be pulled by
the "ofputil_decode_flow_monitor_request" function.
The current patch adds functionality for Flow Monitoring in OpenFlow version
1.4 and later. I will have a separate patch for adding support for OpenFlow
version 1.3 based on the OpenFlow extension pack 2 and would apply the
extension to version 1.1 and 1.2 as well. The 1.3 OpenFlow extension pack 2
is more aligned with the NSXT FLOW_MONITOR implementation.
---
include/openflow/openflow-1.4.h | 94 ++-
include/openvswitch/ofp-monitor.h | 49 +++-
lib/ofp-monitor.c | 534 +++---
lib/ofp-print.c | 10 +-
ofproto/connmgr.c | 98 ---
ofproto/connmgr.h | 11 +-
ofproto/ofproto-provider.h| 7 +-
ofproto/ofproto.c | 124 +
tests/ofproto.at | 381 +++
utilities/ovs-ofctl.8.in | 3 +
utilities/ovs-ofctl.c | 19 +-
11 files changed, 1079 insertions(+), 251 deletions(-)
diff --git a/include/openflow/openflow-1.4.h b/include/openflow/openflow-1.4.h
index 9399950..04d1290 100644
--- a/include/openflow/openflow-1.4.h
+++ b/include/openflow/openflow-1.4.h
@@ -358,7 +358,7 @@ OFP_ASSERT(sizeof(struct ofp14_flow_monitor_request) == 16);
/* Flow monitor commands */
enum ofp14_flow_monitor_command {
-OFPFMC14_ADD = 0, /* New flow monitor. */
+OFPFMC14_ADD = 0,/* New flow monitor. */
OFPFMC14_MODIFY = 1, /* Modify existing flow monitor. */
OFPFMC14_DELETE = 2, /* Delete/cancel existing flow monitor. */
};
@@ -367,18 +367,100 @@ enum ofp14_flow_monitor_command {
enum ofp14_flow_monitor_flags {
/* When to send updates. */
/* Common to NX and OpenFlow 1.4 */
-OFPFMF14_INITIAL = 1 << 0, /* Initially matching flows. */
-OFPFMF14_ADD = 1 << 1, /* New matching flows as they are added. */
-OFPFMF14_REMOVED = 1 << 2, /* Old matching flows as they are removed.
*/
-OFPFMF14_MODIFY = 1 << 3, /* Matching flows as they are changed. */
+OFPFMF14_INITIAL = 1 << 0,/* Initially matching flows. */
+OFPFMF14_ADD = 1 << 1,/* New matching flows as they are added. */
+OFPFMF14_REMOVED = 1 << 2,/* Old matching flows as they are removed. */
+OFPFMF14_MODIFY = 1 << 3, /* Matching flows as they are changed. */
/* What to include in updates. */
/* Common to NX and OpenFlow 1.4 */
OFPFMF14_INSTRUCTIONS = 1 << 4, /* If set, instructions are included. */
OFPFMF14_NO_ABBREV = 1 << 5,/* If set, include own changes in full. */
-/* OpenFlow 1.4 */
+
+/* Only in OpenFlow 1.4, NXFMF_* version does not have NXFMF_ONLY_OWN */
OFPFMF14_ONLY_OWN = 1 << 6, /* If set, don't include other controllers.
*/
};
+/* OFPMP_FLOW_MONITOR reply header.
+ *
+ * The body of an OFPMP_FLOW_MONITOR reply is an array of variable-length
+ * structures, each of which begins with this header. The ’length’ member
+ * may be used to traverse the array, and the ’event’ member may be used to
+ * determine the particular structure.
+ *
+ * Every instance is a multiple of 8 bytes long. */
+struct ofp14_flow_update_header {
+ovs_be16 length; /* Length of this entry. */
+ovs_be16 event; /* One of OFPFME_*. */
+/* ...other data depending on
