| Message ID | 20210508111227.79263-1-vdasari@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] ofp-monitor: Extend Flow Monitoring support for OF 1.0+ with Nicira Extensions | expand |
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@gmail.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050820.html > Signed-off-by: Vasu Dasari <vdasari@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.
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@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@gmail.com> > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050820.html > > Signed-off-by: Vasu Dasari <vdasari@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. >
On Mon, May 10, 2021 at 07:38:08PM -0400, Vasu Dasari wrote: > 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. I agree. For this commit, though, I think that it should only add support up to 1.3, that is, ofp-msgs.h should say 1.0-1.3, not 1.0+. Thanks, Ben.
Ok. Thanks Ben. Will take care of this. -Vasu *Vasu Dasari* On Mon, May 10, 2021 at 7:57 PM Ben Pfaff <blp@ovn.org> wrote: > On Mon, May 10, 2021 at 07:38:08PM -0400, Vasu Dasari wrote: > > 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. > > I agree. For this commit, though, I think that it should only add > support up to 1.3, that is, ofp-msgs.h should say 1.0-1.3, not 1.0+. > > Thanks, > > Ben. >
Hi Ben, Quick comment on test case extension. I have added wrappers as you suggested that code looks good. But I ran into a version specific problem for testcase, "ofproto - flow monitoring". It uses is a ofctl/send <https://github.com/openvswitch/ovs/blob/7100c220e669443aa646513ce6cb241ccf2caf5c/tests/ofproto.at#L4695> command to perform flow-delete operation, which works well for 1.0 as the byte stream is handcrafted for version 1. But for any other version, we need to construct th byte stream and call ofctl/send. So, I left following as it is 4579:AT_SETUP([ofproto - flow monitoring]) 4716:AT_SETUP([ofproto - flow monitoring with !own]) but enhanced these cases to handle all supported versions. 4757:AT_SETUP([ofproto - flow monitoring with out_port]) 4809:AT_SETUP([ofproto - flow monitoring pause and resume]) Here is the latest latest patch <http://patchwork.ozlabs.org/project/openvswitch/patch/20210514193917.81656-1-vdasari@gmail.com/>. Pardon me, I forgot to add v1 to the subject line but I have added the v1 changes in the patch. Thanks -Vasu *Vasu Dasari* On Mon, May 10, 2021 at 8:22 PM Vasu Dasari <vdasari@gmail.com> wrote: > Ok. Thanks Ben. Will take care of this. > > -Vasu > > *Vasu Dasari* > > > On Mon, May 10, 2021 at 7:57 PM Ben Pfaff <blp@ovn.org> wrote: > >> On Mon, May 10, 2021 at 07:38:08PM -0400, Vasu Dasari wrote: >> > 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. >> >> I agree. For this commit, though, I think that it should only add >> support up to 1.3, that is, ofp-msgs.h should say 1.0-1.3, not 1.0+. >> >> Thanks, >> >> Ben. >> >
On Fri, May 14, 2021 at 03:48:53PM -0400, Vasu Dasari wrote: > Hi Ben, > > Quick comment on test case extension. I have added wrappers as you > suggested that code looks good. But I ran into a version specific problem > for testcase, "ofproto - flow monitoring". It uses is a ofctl/send > <https://github.com/openvswitch/ovs/blob/7100c220e669443aa646513ce6cb241ccf2caf5c/tests/ofproto.at#L4695> > command > to perform flow-delete operation, which works well for 1.0 as the byte > stream is handcrafted for version 1. But for any other version, we need to > construct th byte stream and call ofctl/send. So, I left following as it is The easiest way to construct the byte stream is probably to run ovs-ofctl and record what it sends. It shouldn't be very hard to do that, so I'd just go ahead and do it.
Ok Thank you. > On May 14, 2021, at 5:09 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Fri, May 14, 2021 at 03:48:53PM -0400, Vasu Dasari wrote: >> Hi Ben, >> >> Quick comment on test case extension. I have added wrappers as you >> suggested that code looks good. But I ran into a version specific problem >> for testcase, "ofproto - flow monitoring". It uses is a ofctl/send >> <https://github.com/openvswitch/ovs/blob/7100c220e669443aa646513ce6cb241ccf2caf5c/tests/ofproto.at#L4695> >> command >> to perform flow-delete operation, which works well for 1.0 as the byte >> stream is handcrafted for version 1. But for any other version, we need to >> construct th byte stream and call ofctl/send. So, I left following as it is > > The easiest way to construct the byte stream is probably to run > ovs-ofctl and record what it sends. It shouldn't be very hard to do > that, so I'd just go ahead and do it.
Hi Ben, I know you might be busy. Can you please share how to construct byte stream, I will do it. I looked at ovs-ofctl command debug options to see if there is a way to print out the byte OpenFlow packet's byte stream, I could not find one. Thanks -Vasu *Vasu Dasari* On Fri, May 14, 2021 at 5:09 PM Ben Pfaff <blp@ovn.org> wrote: > On Fri, May 14, 2021 at 03:48:53PM -0400, Vasu Dasari wrote: > > Hi Ben, > > > > Quick comment on test case extension. I have added wrappers as you > > suggested that code looks good. But I ran into a version specific problem > > for testcase, "ofproto - flow monitoring". It uses is a ofctl/send > > < > https://github.com/openvswitch/ovs/blob/7100c220e669443aa646513ce6cb241ccf2caf5c/tests/ofproto.at#L4695 > > > > command > > to perform flow-delete operation, which works well for 1.0 as the byte > > stream is handcrafted for version 1. But for any other version, we need > to > > construct th byte stream and call ofctl/send. So, I left following as it > is > > The easiest way to construct the byte stream is probably to run > ovs-ofctl and record what it sends. It shouldn't be very hard to do > that, so I'd just go ahead and do it. >
You can apply a patch like this to get it to log hex:
diff --git a/lib/vconn.c b/lib/vconn.c
index 7415e6291f6f..c626c35f9792 100644
--- a/lib/vconn.c
+++ b/lib/vconn.c
@@ -685,7 +685,7 @@ do_send(struct vconn *vconn, struct ofpbuf *msg)
COVERAGE_INC(vconn_sent);
retval = (vconn->vclass->send)(vconn, msg);
} else {
- char *s = ofp_to_string(msg->data, msg->size, NULL, NULL, 1);
+ char *s = ofp_to_string(msg->data, msg->size, NULL, NULL, 5);
retval = (vconn->vclass->send)(vconn, msg);
if (retval != EAGAIN) {
VLOG_DBG_RL(&ofmsg_rl, "%s: sent (%s): %s",
On Wed, Jun 09, 2021 at 08:17:18PM -0400, Vasu Dasari wrote:
> Hi Ben,
>
> I know you might be busy. Can you please share how to construct byte
> stream, I will do it.
>
> I looked at ovs-ofctl command debug options to see if there is a way to
> print out the byte OpenFlow packet's byte stream, I could not find one.
>
> Thanks
> -Vasu
>
> *Vasu Dasari*
>
>
> On Fri, May 14, 2021 at 5:09 PM Ben Pfaff <blp@ovn.org> wrote:
>
> > On Fri, May 14, 2021 at 03:48:53PM -0400, Vasu Dasari wrote:
> > > Hi Ben,
> > >
> > > Quick comment on test case extension. I have added wrappers as you
> > > suggested that code looks good. But I ran into a version specific problem
> > > for testcase, "ofproto - flow monitoring". It uses is a ofctl/send
> > > <
> > https://github.com/openvswitch/ovs/blob/7100c220e669443aa646513ce6cb241ccf2caf5c/tests/ofproto.at#L4695
> > >
> > > command
> > > to perform flow-delete operation, which works well for 1.0 as the byte
> > > stream is handcrafted for version 1. But for any other version, we need
> > to
> > > construct th byte stream and call ofctl/send. So, I left following as it
> > is
> >
> > The easiest way to construct the byte stream is probably to run
> > ovs-ofctl and record what it sends. It shouldn't be very hard to do
> > that, so I'd just go ahead and do it.
> >
Thanks Ben. Will try this out. -Vasu *Vasu Dasari* On Thu, Jun 10, 2021 at 5:32 PM Ben Pfaff <blp@ovn.org> wrote: > You can apply a patch like this to get it to log hex: > > diff --git a/lib/vconn.c b/lib/vconn.c > index 7415e6291f6f..c626c35f9792 100644 > --- a/lib/vconn.c > +++ b/lib/vconn.c > @@ -685,7 +685,7 @@ do_send(struct vconn *vconn, struct ofpbuf *msg) > COVERAGE_INC(vconn_sent); > retval = (vconn->vclass->send)(vconn, msg); > } else { > - char *s = ofp_to_string(msg->data, msg->size, NULL, NULL, 1); > + char *s = ofp_to_string(msg->data, msg->size, NULL, NULL, 5); > retval = (vconn->vclass->send)(vconn, msg); > if (retval != EAGAIN) { > VLOG_DBG_RL(&ofmsg_rl, "%s: sent (%s): %s", > > > On Wed, Jun 09, 2021 at 08:17:18PM -0400, Vasu Dasari wrote: > > Hi Ben, > > > > I know you might be busy. Can you please share how to construct byte > > stream, I will do it. > > > > I looked at ovs-ofctl command debug options to see if there is a way to > > print out the byte OpenFlow packet's byte stream, I could not find one. > > > > Thanks > > -Vasu > > > > *Vasu Dasari* > > > > > > On Fri, May 14, 2021 at 5:09 PM Ben Pfaff <blp@ovn.org> wrote: > > > > > On Fri, May 14, 2021 at 03:48:53PM -0400, Vasu Dasari wrote: > > > > Hi Ben, > > > > > > > > Quick comment on test case extension. I have added wrappers as you > > > > suggested that code looks good. But I ran into a version specific > problem > > > > for testcase, "ofproto - flow monitoring". It uses is a ofctl/send > > > > < > > > > https://github.com/openvswitch/ovs/blob/7100c220e669443aa646513ce6cb241ccf2caf5c/tests/ofproto.at#L4695 > > > > > > > > command > > > > to perform flow-delete operation, which works well for 1.0 as the > byte > > > > stream is handcrafted for version 1. But for any other version, we > need > > > to > > > > construct th byte stream and call ofctl/send. So, I left following > as it > > > is > > > > > > The easiest way to construct the byte stream is probably to run > > > ovs-ofctl and record what it sends. It shouldn't be very hard to do > > > that, so I'd just go ahead and do it. > > > >
diff --git a/include/openvswitch/ofp-monitor.h b/include/openvswitch/ofp-monitor.h index 237cef85e..835efd0f3 100644 --- a/include/openvswitch/ofp-monitor.h +++ b/include/openvswitch/ofp-monitor.h @@ -70,7 +70,8 @@ struct ofputil_flow_monitor_request { int ofputil_decode_flow_monitor_request(struct ofputil_flow_monitor_request *, struct ofpbuf *msg); void ofputil_append_flow_monitor_request( - const struct ofputil_flow_monitor_request *, struct ofpbuf *msg); + const struct ofputil_flow_monitor_request *, struct ofpbuf *msg, + enum ofputil_protocol protocol); void ofputil_flow_monitor_request_format( struct ds *, const struct ofputil_flow_monitor_request *, const struct ofputil_port_map *, const struct ofputil_table_map *); @@ -103,7 +104,8 @@ struct ofputil_flow_update { int ofputil_decode_flow_update(struct ofputil_flow_update *, struct ofpbuf *msg, struct ofpbuf *ofpacts); -void ofputil_start_flow_update(struct ovs_list *replies); +void ofputil_start_flow_update(struct ovs_list *replies, + enum ofputil_protocol protocol); void ofputil_append_flow_update(const struct ofputil_flow_update *, struct ovs_list *replies, const struct tun_table *); @@ -114,7 +116,8 @@ void ofputil_flow_update_format(struct ds *, /* Abstract nx_flow_monitor_cancel. */ uint32_t ofputil_decode_flow_monitor_cancel(const struct ofp_header *); -struct ofpbuf *ofputil_encode_flow_monitor_cancel(uint32_t id); +struct ofpbuf *ofputil_encode_flow_monitor_cancel( + uint32_t id, enum ofputil_protocol protocol); struct ofputil_requestforward { ovs_be32 xid; diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h index 8457bc7d0..be1bc2745 100644 --- a/include/openvswitch/ofp-msgs.h +++ b/include/openvswitch/ofp-msgs.h @@ -453,12 +453,12 @@ enum ofpraw { /* OFPST 1.4+ (16): uint8_t[8][]. */ OFPRAW_OFPST14_FLOW_MONITOR_REQUEST, - /* NXST 1.0 (2): uint8_t[8][]. */ + /* NXST 1.0+ (2): uint8_t[8][]. */ OFPRAW_NXST_FLOW_MONITOR_REQUEST, /* OFPST 1.4+ (16): uint8_t[8][]. */ OFPRAW_OFPST14_FLOW_MONITOR_REPLY, - /* NXST 1.0 (2): uint8_t[8][]. */ + /* NXST 1.0+ (2): uint8_t[8][]. */ OFPRAW_NXST_FLOW_MONITOR_REPLY, /* Nicira extension messages. diff --git a/lib/ofp-monitor.c b/lib/ofp-monitor.c index e12fa6d2b..51f01b100 100644 --- a/lib/ofp-monitor.c +++ b/lib/ofp-monitor.c @@ -386,14 +386,16 @@ ofputil_decode_flow_monitor_request(struct ofputil_flow_monitor_request *rq, void ofputil_append_flow_monitor_request( - const struct ofputil_flow_monitor_request *rq, struct ofpbuf *msg) + const struct ofputil_flow_monitor_request *rq, struct ofpbuf *msg, + enum ofputil_protocol protocol) { struct nx_flow_monitor_request *nfmr; size_t start_ofs; int match_len; + enum ofp_version version = ofputil_protocol_to_ofp_version(protocol); if (!msg->size) { - ofpraw_put(OFPRAW_NXST_FLOW_MONITOR_REQUEST, OFP10_VERSION, msg); + ofpraw_put(OFPRAW_NXST_FLOW_MONITOR_REQUEST, version, msg); } start_ofs = msg->size; @@ -517,9 +519,6 @@ parse_flow_monitor_request__(struct ofputil_flow_monitor_request *fmr, if (error) { return error; } - /* Flow Monitor is supported in OpenFlow 1.0 or can be further reduced - * to a few 1.0 flavors by a match field. */ - *usable_protocols &= OFPUTIL_P_OF10_ANY; } return NULL; } @@ -661,23 +660,26 @@ ofputil_decode_flow_monitor_cancel(const struct ofp_header *oh) } struct ofpbuf * -ofputil_encode_flow_monitor_cancel(uint32_t id) +ofputil_encode_flow_monitor_cancel(uint32_t id, enum ofputil_protocol protocol) { struct nx_flow_monitor_cancel *nfmc; + enum ofp_version version = ofputil_protocol_to_ofp_version(protocol); struct ofpbuf *msg; - msg = ofpraw_alloc(OFPRAW_NXT_FLOW_MONITOR_CANCEL, OFP10_VERSION, 0); + msg = ofpraw_alloc(OFPRAW_NXT_FLOW_MONITOR_CANCEL, version, 0); nfmc = ofpbuf_put_uninit(msg, sizeof *nfmc); nfmc->id = htonl(id); return msg; } void -ofputil_start_flow_update(struct ovs_list *replies) +ofputil_start_flow_update(struct ovs_list *replies, + enum ofputil_protocol protocol) { struct ofpbuf *msg; + enum ofp_version version = ofputil_protocol_to_ofp_version(protocol); - msg = ofpraw_alloc_xid(OFPRAW_NXST_FLOW_MONITOR_REPLY, OFP10_VERSION, + msg = ofpraw_alloc_xid(OFPRAW_NXST_FLOW_MONITOR_REPLY, version, htonl(0), 1024); ovs_list_init(replies); diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index fa8f6cd0e..c14834f84 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -2193,7 +2193,8 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, if (flags) { if (ovs_list_is_empty(&ofconn->updates)) { - ofputil_start_flow_update(&ofconn->updates); + ofputil_start_flow_update(&ofconn->updates, + ofconn_get_protocol(ofconn)); ofconn->sent_abbrev_update = false; } @@ -2243,6 +2244,7 @@ ofmonitor_flush(struct connmgr *mgr) OVS_REQUIRES(ofproto_mutex) { struct ofconn *ofconn; + enum ofputil_protocol protocol; if (!mgr) { return; @@ -2260,8 +2262,10 @@ ofmonitor_flush(struct connmgr *mgr) && rconn_packet_counter_n_bytes(counter) > 128 * 1024) { COVERAGE_INC(ofmonitor_pause); ofconn->monitor_paused = monitor_seqno++; + protocol = ofconn_get_protocol(ofconn); struct ofpbuf *pause = ofpraw_alloc_xid( - OFPRAW_NXT_FLOW_MONITOR_PAUSED, OFP10_VERSION, htonl(0), 0); + OFPRAW_NXT_FLOW_MONITOR_PAUSED, + ofputil_protocol_to_ofp_version(protocol), htonl(0), 0); ofconn_send(ofconn, pause, counter); } } @@ -2271,6 +2275,7 @@ static void ofmonitor_resume(struct ofconn *ofconn) OVS_REQUIRES(ofproto_mutex) { + enum ofputil_protocol protocol; struct rule_collection rules; rule_collection_init(&rules); @@ -2280,10 +2285,13 @@ ofmonitor_resume(struct ofconn *ofconn) } struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs); - ofmonitor_compose_refresh_updates(&rules, &msgs); + ofmonitor_compose_refresh_updates(&rules, &msgs, + ofconn_get_protocol(ofconn)); - struct ofpbuf *resumed = ofpraw_alloc_xid(OFPRAW_NXT_FLOW_MONITOR_RESUMED, - OFP10_VERSION, htonl(0), 0); + protocol = ofconn_get_protocol(ofconn); + struct ofpbuf *resumed = ofpraw_alloc_xid( + OFPRAW_NXT_FLOW_MONITOR_RESUMED, + ofputil_protocol_to_ofp_version(protocol), htonl(0), 0); ovs_list_push_back(&msgs, &resumed->list_node); ofconn_send_replies(ofconn, &msgs); diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index e299386c7..56fdc3504 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -199,7 +199,8 @@ void ofmonitor_collect_resume_rules(struct ofmonitor *, uint64_t seqno, struct rule_collection *) OVS_REQUIRES(ofproto_mutex); void ofmonitor_compose_refresh_updates(struct rule_collection *rules, - struct ovs_list *msgs) + struct ovs_list *msgs, + enum ofputil_protocol protocol) OVS_REQUIRES(ofproto_mutex); void connmgr_send_table_status(struct connmgr *, diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index b91517cd2..fdae5c462 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6351,7 +6351,8 @@ static void ofproto_compose_flow_refresh_update(const struct rule *rule, enum nx_flow_monitor_flags flags, struct ovs_list *msgs, - const struct tun_table *tun_table) + const struct tun_table *tun_table, + enum ofputil_protocol protocol) OVS_REQUIRES(ofproto_mutex) { const struct rule_actions *actions; @@ -6374,14 +6375,15 @@ ofproto_compose_flow_refresh_update(const struct rule *rule, fu.ofpacts_len = actions ? actions->ofpacts_len : 0; if (ovs_list_is_empty(msgs)) { - ofputil_start_flow_update(msgs); + ofputil_start_flow_update(msgs, protocol); } ofputil_append_flow_update(&fu, msgs, tun_table); } void ofmonitor_compose_refresh_updates(struct rule_collection *rules, - struct ovs_list *msgs) + struct ovs_list *msgs, + enum ofputil_protocol protocol) OVS_REQUIRES(ofproto_mutex) { struct rule *rule; @@ -6391,7 +6393,7 @@ ofmonitor_compose_refresh_updates(struct rule_collection *rules, rule->monitor_flags = 0; ofproto_compose_flow_refresh_update(rule, flags, msgs, - ofproto_get_tun_tab(rule->ofproto)); + ofproto_get_tun_tab(rule->ofproto), protocol); } } @@ -6555,7 +6557,8 @@ handle_flow_monitor_request(struct ofconn *ofconn, const struct ovs_list *msgs) struct ovs_list replies; ofpmp_init(&replies, ofpbuf_from_list(ovs_list_back(msgs))->header); - ofmonitor_compose_refresh_updates(&rules, &replies); + ofmonitor_compose_refresh_updates(&rules, &replies, + ofconn_get_protocol(ofconn)); ovs_mutex_unlock(&ofproto_mutex); rule_collection_destroy(&rules); diff --git a/tests/ofproto.at b/tests/ofproto.at index 08c0a20b6..99b01a00e 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -4911,6 +4911,7 @@ NXT_FLOW_MONITOR_RESUMED: OVS_VSWITCHD_STOP AT_CLEANUP +# Test to show flow monitoring support on different OpenFlow protocols AT_SETUP([ofproto - flow monitoring usable protocols]) AT_KEYWORDS([monitor]) @@ -4920,14 +4921,22 @@ on_exit 'kill `cat ovs-ofctl.pid`' ovs-ofctl -OOpenFlow14 monitor br0 watch:udp,udp_dst=8 --detach --no-chdir --pidfile >monitor.log 2>&1 AT_CAPTURE_FILE([monitor.log]) -# ovs-ofctl should exit because monitor is not supported in OpenFlow 1.4 -OVS_WAIT_UNTIL([grep "ovs-ofctl: none of the usable flow formats (OpenFlow10,NXM) is among the allowed flow formats (OXM-OpenFlow14)" monitor.log]) +# Wait till reply comes backs with OF 1.4 +OVS_WAIT_UNTIL([grep "NXST_FLOW_MONITOR reply (OF1.4)" monitor.log]) +ovs-appctl -t ovs-ofctl exit + +# Make sure protocol type in messages from vswitchd, matches that of requested protocol +ovs-ofctl -OOpenFlow13 monitor br0 watch:sctp,sctp_dst=9 --detach --no-chdir --pidfile >monitor.log 2>&1 +ovs-ofctl add-flow br0 sctp,sctp_dst=9,action=normal -# check that only NXM flag is returned as usable protocols for sctp_dst -# and ovs-ofctl should exit since monitor is not supported in OpenFlow 1.4 -ovs-ofctl -OOpenFlow14 monitor br0 watch:sctp,sctp_dst=9 --detach --no-chdir --pidfile >monitor.log 2>&1 -OVS_WAIT_UNTIL([grep "ovs-ofctl: none of the usable flow formats (NXM) is among the allowed flow formats (OXM-OpenFlow14)" monitor.log]) +OVS_WAIT_UNTIL([grep "event=ADDED " monitor.log]) +AT_CHECK([sed 's/ (xid=0x[[1-9a-fA-F]][[0-9a-fA-F]]*)//' monitor.log], [0], [dnl +NXST_FLOW_MONITOR reply (OF1.3): +NXST_FLOW_MONITOR reply (OF1.3) (xid=0x0): + event=ADDED table=0 cookie=0 sctp,tp_dst=9 actions=NORMAL +]) +OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) OVS_VSWITCHD_STOP AT_CLEANUP diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index 62059e962..d551f6639 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -2239,6 +2239,7 @@ ofctl_monitor(struct ovs_cmdl_context *ctx) { struct vconn *vconn; int i; + enum ofputil_protocol protocol; enum ofputil_protocol usable_protocols; /* If the user wants the invalid_ttl_to_controller feature, limit the @@ -2263,7 +2264,7 @@ ofctl_monitor(struct ovs_cmdl_context *ctx) } } - open_vconn(ctx->argv[1], &vconn); + protocol = open_vconn(ctx->argv[1], &vconn); bool resume_continuations = false; for (i = 2; i < ctx->argc; i++) { const char *arg = ctx->argv[i]; @@ -2298,7 +2299,7 @@ ofctl_monitor(struct ovs_cmdl_context *ctx) } msg = ofpbuf_new(0); - ofputil_append_flow_monitor_request(&fmr, msg); + ofputil_append_flow_monitor_request(&fmr, msg, protocol); dump_transaction(vconn, msg); fflush(stdout); } else if (!strcmp(arg, "resume")) {