| Message ID | 1474620783-96388-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Sep 23, 2016 at 01:53:00AM -0700, Justin Pettit wrote: > Signed-off-by: Justin Pettit <jpettit@ovn.org> ct_dpif_flush() and dpif_ct_flush() are almost identical. Why do we need both? Acked-by: Ben Pfaff <blp@ovn.org>
> On Sep 23, 2016, at 8:42 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Fri, Sep 23, 2016 at 01:53:00AM -0700, Justin Pettit wrote: >> Signed-off-by: Justin Pettit <jpettit@ovn.org> > > ct_dpif_flush() and dpif_ct_flush() are almost identical. Why do we > need both? > > Acked-by: Ben Pfaff <blp@ovn.org> Just an oversight. I'll add my logging changes to ct_dpif_flush(), since the new unit tests depend on them. Thanks, --Justin
On 23 September 2016 at 01:53, Justin Pettit <jpettit@ovn.org> wrote: > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > NEWS | 1 + > debian/changelog | 10 ++++++++++ > include/openflow/nicira-ext.h | 9 +++++++++ > include/openvswitch/ofp-msgs.h | 4 ++++ > lib/dpif.c | 14 ++++++++++++++ > lib/dpif.h | 1 + > lib/ofp-print.c | 10 ++++++++++ > lib/ofp-util.c | 1 + > lib/rconn.c | 1 + > ofproto/ofproto-dpif.c | 9 +++++++++ > ofproto/ofproto-provider.h | 7 +++++++ > ofproto/ofproto.c | 23 +++++++++++++++++++++++ > tests/ofp-print.at | 10 ++++++++++ > tests/ovs-ofctl.at | 13 +++++++++++++ > utilities/ovs-ofctl.8.in | 6 ++++++ > utilities/ovs-ofctl.c | 25 +++++++++++++++++++++++++ > 16 files changed, 144 insertions(+) > > diff --git a/NEWS b/NEWS > index bb32c47..dba1950 100644 > --- a/NEWS > +++ b/NEWS > @@ -71,6 +71,7 @@ v2.6.0 - xx xxx xxxx > already expected to work properly in cases where the switch can > not buffer packets, so this change should not affect existing > users. > + * New OpenFlow extension NXT_CT_FLUSH_ZONE to flush conntrack zones. > - Improved OpenFlow version compatibility for actions: > * New OpenFlow extension to support the "group" action in OpenFlow 1.0. > * OpenFlow 1.0 "enqueue" action now properly translated to OpenFlow 1.1+. > diff --git a/debian/changelog b/debian/changelog > index d73e636..7f38d53 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -33,6 +33,16 @@ openvswitch (2.6.0-1) unstable; urgency=low > packet to size M bytes when outputting to port N. > * New command OFPGC_ADD_OR_MOD for OFPT_GROUP_MOD message that adds a > new group or modifies an existing groups > + * The optional OpenFlow packet buffering feature is deprecated in > + this release, and will be removed in the next OVS release > + (2.7). After the change OVS always sends the 'buffer_id' as > + 0xffffffff in packet-in messages and will send an error > + response if any other value of this field is included in > + packet-out and flow mod sent by a controller. Controllers are > + already expected to work properly in cases where the switch can > + not buffer packets, so this change should not affect existing > + users. > + * New OpenFlow extension NXT_CT_FLUSH_ZONE to flush conntrack zones. > - Improved OpenFlow version compatibility for actions: > * New OpenFlow extension to support the "group" action in OpenFlow 1.0. > * OpenFlow 1.0 "enqueue" action now properly translated to OpenFlow 1.1+. > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > index 5ab026c..9d53623 100644 > --- a/include/openflow/nicira-ext.h > +++ b/include/openflow/nicira-ext.h > @@ -1128,5 +1128,14 @@ struct nx_tlv_table_reply { > from the length field in the header. */ > }; > OFP_ASSERT(sizeof(struct nx_tlv_table_reply) == 16); > + > +/* NXT_CT_FLUSH_ZONE. > + * > + * Flushes the connection tracking table. */ > +struct nx_zone_id { > + uint8_t zero[6]; /* Must be zero. */ > + ovs_be16 zone_id; /* Connection tracking zone. */ > +}; > +OFP_ASSERT(sizeof(struct nx_zone_id) == 8); Can we use OXMs for this? I could foresee people wanting to apply additional constraints such as mark, label, and potentially other fields in future. We can reject the others for now, but since this is an OpenFlow (extension) API change it'd be good to provide something extensible.
> On Sep 23, 2016, at 10:10 AM, Joe Stringer <joe@ovn.org> wrote: > > On 23 September 2016 at 01:53, Justin Pettit <jpettit@ovn.org> wrote: >> >> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h >> index 5ab026c..9d53623 100644 >> --- a/include/openflow/nicira-ext.h >> +++ b/include/openflow/nicira-ext.h >> @@ -1128,5 +1128,14 @@ struct nx_tlv_table_reply { >> from the length field in the header. */ >> }; >> OFP_ASSERT(sizeof(struct nx_tlv_table_reply) == 16); >> + >> +/* NXT_CT_FLUSH_ZONE. >> + * >> + * Flushes the connection tracking table. */ >> +struct nx_zone_id { >> + uint8_t zero[6]; /* Must be zero. */ >> + ovs_be16 zone_id; /* Connection tracking zone. */ >> +}; >> +OFP_ASSERT(sizeof(struct nx_zone_id) == 8); > > Can we use OXMs for this? I could foresee people wanting to apply > additional constraints such as mark, label, and potentially other > fields in future. We can reject the others for now, but since this is > an OpenFlow (extension) API change it'd be good to provide something > extensible. It's a good suggestion. That would be more of a selective delete than a flush command, which is more general. However, since we're up against the 2.6 release and it's easy enough to add another extension, I'd prefer to commit this as-is. We're going to need to build out the OpenFlow interfaces for handling individual conntrack entries anyway, so let's just add the selective deletes along with the dumping commands at the same time. --Justin
On 23 September 2016 at 12:13, Justin Pettit <jpettit@ovn.org> wrote: > >> On Sep 23, 2016, at 10:10 AM, Joe Stringer <joe@ovn.org> wrote: >> >> On 23 September 2016 at 01:53, Justin Pettit <jpettit@ovn.org> wrote: >>> >>> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h >>> index 5ab026c..9d53623 100644 >>> --- a/include/openflow/nicira-ext.h >>> +++ b/include/openflow/nicira-ext.h >>> @@ -1128,5 +1128,14 @@ struct nx_tlv_table_reply { >>> from the length field in the header. */ >>> }; >>> OFP_ASSERT(sizeof(struct nx_tlv_table_reply) == 16); >>> + >>> +/* NXT_CT_FLUSH_ZONE. >>> + * >>> + * Flushes the connection tracking table. */ >>> +struct nx_zone_id { >>> + uint8_t zero[6]; /* Must be zero. */ >>> + ovs_be16 zone_id; /* Connection tracking zone. */ >>> +}; >>> +OFP_ASSERT(sizeof(struct nx_zone_id) == 8); >> >> Can we use OXMs for this? I could foresee people wanting to apply >> additional constraints such as mark, label, and potentially other >> fields in future. We can reject the others for now, but since this is >> an OpenFlow (extension) API change it'd be good to provide something >> extensible. > > It's a good suggestion. That would be more of a selective delete than a flush command, which is more general. However, since we're up against the 2.6 release and it's easy enough to add another extension, I'd prefer to commit this as-is. We're going to need to build out the OpenFlow interfaces for handling individual conntrack entries anyway, so let's just add the selective deletes along with the dumping commands at the same time. A message type with the semantics of "delete with 0 or more constraints" would allow not just /more/ selective deletion, but also /less/ selective deletion - eg, flush all zones. We already have the same functionality of this patch with "ovs-appctl dpctl/flush-conntrack", so I think it's really unfortunate to be defining such a specific message type in OpenFlow as well when we know we'll have to go back and do it properly later.
diff --git a/NEWS b/NEWS index bb32c47..dba1950 100644 --- a/NEWS +++ b/NEWS @@ -71,6 +71,7 @@ v2.6.0 - xx xxx xxxx already expected to work properly in cases where the switch can not buffer packets, so this change should not affect existing users. + * New OpenFlow extension NXT_CT_FLUSH_ZONE to flush conntrack zones. - Improved OpenFlow version compatibility for actions: * New OpenFlow extension to support the "group" action in OpenFlow 1.0. * OpenFlow 1.0 "enqueue" action now properly translated to OpenFlow 1.1+. diff --git a/debian/changelog b/debian/changelog index d73e636..7f38d53 100644 --- a/debian/changelog +++ b/debian/changelog @@ -33,6 +33,16 @@ openvswitch (2.6.0-1) unstable; urgency=low packet to size M bytes when outputting to port N. * New command OFPGC_ADD_OR_MOD for OFPT_GROUP_MOD message that adds a new group or modifies an existing groups + * The optional OpenFlow packet buffering feature is deprecated in + this release, and will be removed in the next OVS release + (2.7). After the change OVS always sends the 'buffer_id' as + 0xffffffff in packet-in messages and will send an error + response if any other value of this field is included in + packet-out and flow mod sent by a controller. Controllers are + already expected to work properly in cases where the switch can + not buffer packets, so this change should not affect existing + users. + * New OpenFlow extension NXT_CT_FLUSH_ZONE to flush conntrack zones. - Improved OpenFlow version compatibility for actions: * New OpenFlow extension to support the "group" action in OpenFlow 1.0. * OpenFlow 1.0 "enqueue" action now properly translated to OpenFlow 1.1+. diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index 5ab026c..9d53623 100644 --- a/include/openflow/nicira-ext.h +++ b/include/openflow/nicira-ext.h @@ -1128,5 +1128,14 @@ struct nx_tlv_table_reply { from the length field in the header. */ }; OFP_ASSERT(sizeof(struct nx_tlv_table_reply) == 16); + +/* NXT_CT_FLUSH_ZONE. + * + * Flushes the connection tracking table. */ +struct nx_zone_id { + uint8_t zero[6]; /* Must be zero. */ + ovs_be16 zone_id; /* Connection tracking zone. */ +}; +OFP_ASSERT(sizeof(struct nx_zone_id) == 8); #endif /* openflow/nicira-ext.h */ diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h index 8dab858..ffb88b3 100644 --- a/include/openvswitch/ofp-msgs.h +++ b/include/openvswitch/ofp-msgs.h @@ -468,6 +468,9 @@ enum ofpraw { /* NXT 1.0+ (28): uint8_t[8][]. */ OFPRAW_NXT_RESUME, + /* NXT 1.0+ (29): struct nx_zone_id. */ + OFPRAW_NXT_CT_FLUSH_ZONE, + /* NXST 1.0+ (3): void. */ OFPRAW_NXST_IPFIX_BRIDGE_REQUEST, @@ -707,6 +710,7 @@ enum ofptype { OFPTYPE_IPFIX_BRIDGE_STATS_REPLY, /* OFPRAW_NXST_IPFIX_BRIDGE_REPLY */ OFPTYPE_IPFIX_FLOW_STATS_REQUEST, /* OFPRAW_NXST_IPFIX_FLOW_REQUEST */ OFPTYPE_IPFIX_FLOW_STATS_REPLY, /* OFPRAW_NXST_IPFIX_FLOW_REPLY */ + OFPTYPE_CT_FLUSH_ZONE, /* OFPRAW_NXT_CT_FLUSH_ZONE. */ /* Flow monitor extension. */ OFPTYPE_FLOW_MONITOR_CANCEL, /* OFPRAW_NXT_FLOW_MONITOR_CANCEL. */ diff --git a/lib/dpif.c b/lib/dpif.c index 53958c5..3fcef77 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1760,3 +1760,17 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) { return dpif_is_netdev(dpif); } + +void +dpif_ct_flush(struct dpif *dpif, const uint16_t *zone) +{ + if (zone) { + VLOG_DBG("%s: ct_flush: %"PRIu16, dpif_name(dpif), *zone); + } else { + VLOG_DBG("%s: ct_flush: <all>", dpif_name(dpif)); + } + + if (dpif->dpif_class->ct_flush) { + dpif->dpif_class->ct_flush(dpif, zone); + } +} diff --git a/lib/dpif.h b/lib/dpif.h index a7c5097..a6ead6e 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -859,6 +859,7 @@ int dpif_queue_to_priority(const struct dpif *, uint32_t queue_id, char *dpif_get_dp_version(const struct dpif *); bool dpif_supports_tnl_push_pop(const struct dpif *); +void dpif_ct_flush(struct dpif *, const uint16_t *zone); #ifdef __cplusplus } #endif diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 917dce8..0a68551 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -3324,6 +3324,12 @@ ofp_print_nxst_ipfix_flow_reply(struct ds *string, const struct ofp_header *oh) } } +static void +ofp_print_nxt_ct_flush_zone(struct ds *string, const struct nx_zone_id *nzi) +{ + ds_put_format(string, " zone_id=%"PRIu16, ntohs(nzi->zone_id)); +} + static void ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw, @@ -3625,6 +3631,10 @@ ofp_to_string__(const struct ofp_header *oh, enum ofpraw raw, case OFPTYPE_IPFIX_FLOW_STATS_REPLY: ofp_print_nxst_ipfix_flow_reply(string, oh); break; + + case OFPTYPE_CT_FLUSH_ZONE: + ofp_print_nxt_ct_flush_zone(string, ofpmsg_body(oh)); + break; } } diff --git a/lib/ofp-util.c b/lib/ofp-util.c index e2ccd6c..0445968 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -9998,6 +9998,7 @@ ofputil_is_bundlable(enum ofptype type) case OFPTYPE_IPFIX_BRIDGE_STATS_REPLY: case OFPTYPE_IPFIX_FLOW_STATS_REQUEST: case OFPTYPE_IPFIX_FLOW_STATS_REPLY: + case OFPTYPE_CT_FLUSH_ZONE: break; } diff --git a/lib/rconn.c b/lib/rconn.c index 51e1b1b..0c1812a 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -1430,6 +1430,7 @@ is_admitted_msg(const struct ofpbuf *b) case OFPTYPE_IPFIX_BRIDGE_STATS_REPLY: case OFPTYPE_IPFIX_FLOW_STATS_REQUEST: case OFPTYPE_IPFIX_FLOW_STATS_REPLY: + case OFPTYPE_CT_FLUSH_ZONE: default: return true; } diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 0282ebd..cc71673 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4656,6 +4656,14 @@ get_datapath_version(const struct ofproto *ofproto_) return ofproto->backer->dp_version_string; } +static void +ct_flush(const struct ofproto *ofproto_, const uint16_t *zone) +{ + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); + + dpif_ct_flush(ofproto->backer->dpif, zone); +} + static bool set_frag_handling(struct ofproto *ofproto_, enum ofputil_frag_handling frag_handling) @@ -5924,4 +5932,5 @@ const struct ofproto_class ofproto_dpif_class = { NULL, /* group_modify */ group_get_stats, /* group_get_stats */ get_datapath_version, /* get_datapath_version */ + ct_flush, /* ct_flush */ }; diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 6a523b4..bc5098a 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1811,6 +1811,13 @@ struct ofproto_class { * This function should be NULL if an implementation does not support it. */ const char *(*get_datapath_version)(const struct ofproto *); + +/* ## ------------------- ## */ +/* ## Connection tracking ## */ +/* ## ------------------- ## */ + /* Flushes the connection tracking tables. If 'zone' is not NULL, + * only deletes connections in '*zone'. */ + void (*ct_flush)(const struct ofproto *, const uint16_t *zone); }; extern const struct ofproto_class ofproto_dpif_class; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 93a24be..d601f71 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -879,6 +879,26 @@ handle_ipfix_flow_stats_request(struct ofconn *ofconn, return error; } +static enum ofperr +handle_nxt_ct_flush_zone(struct ofconn *ofconn, const struct ofp_header *oh) +{ + struct ofproto *ofproto = ofconn_get_ofproto(ofconn); + const struct nx_zone_id *nzi = ofpmsg_body(oh); + + if (!is_all_zeros(nzi->zero, sizeof nzi->zero)) { + return OFPERR_NXBRC_MUST_BE_ZERO; + } + + uint16_t zone = ntohs(nzi->zone_id); + if (ofproto->ofproto_class->ct_flush) { + ofproto->ofproto_class->ct_flush(ofproto, &zone); + } else { + return EOPNOTSUPP; + } + + return 0; +} + void ofproto_set_flow_restore_wait(bool flow_restore_wait_db) { @@ -7930,6 +7950,9 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg) case OFPTYPE_IPFIX_FLOW_STATS_REQUEST: return handle_ipfix_flow_stats_request(ofconn, oh); + case OFPTYPE_CT_FLUSH_ZONE: + return handle_nxt_ct_flush_zone(ofconn, oh); + case OFPTYPE_HELLO: case OFPTYPE_ERROR: case OFPTYPE_FEATURES_REPLY: diff --git a/tests/ofp-print.at b/tests/ofp-print.at index 6c7433d..6946438 100644 --- a/tests/ofp-print.at +++ b/tests/ofp-print.at @@ -3629,3 +3629,13 @@ NXST_IPFIX_FLOW reply (xid=0x2): 2 ids pkts errs=160, ipv4 errs=68719476738, ipv6 errs=3, tx errs=5 ]) AT_CLEANUP + +AT_SETUP([NXT_CT_FLUSH_ZONE]) +AT_KEYWORDS([ofp-print]) +AT_CHECK([ovs-ofctl ofp-print "\ +01 04 00 18 00 00 00 03 00 00 23 20 00 00 00 1d \ +00 00 00 00 00 00 00 0d \ +"], [0], [dnl +NXT_CT_FLUSH_ZONE (xid=0x3): zone_id=13 +]) +AT_CLEANUP diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index 8747a06..719736f 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -3086,3 +3086,16 @@ vconn|DBG|unix: sent (Success): OFPST_FLOW reply (OF1.4): OVS_VSWITCHD_STOP AT_CLEANUP + + +AT_SETUP([ovs-ofctl ct-flush-zone]) +OVS_VSWITCHD_START + +AT_CHECK([ovs-appctl vlog/set dpif:dbg]) +AT_CHECK([ovs-ofctl ct-flush-zone br0 123]) + +OVS_WAIT_UNTIL([grep -q "|dpif|DBG|.*ct_flush:" ovs-vswitchd.log]) +AT_CHECK([grep -q "dpif|DBG|.*ct_flush: 123" ovs-vswitchd.log]) + +OVS_VSWITCHD_STOP +AT_CLEANUP diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 6ab9638..c112e6a 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -275,6 +275,12 @@ flow based IPFIX and collector set ids. This command uses an Open vSwitch extension that is only in Open vSwitch 2.6 and later. . +.IP "\fBct\-flush\-zone \fIswitch zone\fR +Flushes the connection tracking entries in \fIzone\fR on \fIswitch\fR. +.IP +This command uses an Open vSwitch extension that is only in Open +vSwitch 2.6 and later. +. .SS "OpenFlow 1.1+ Group Table Commands" . The following commands work only with switches that support OpenFlow diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index f9138e9..5a43da4 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -450,6 +450,7 @@ usage(void) " dump-tlv-map SWITCH print TLV option mappings\n" " dump-ipfix-bridge SWITCH print ipfix stats of bridge\n" " dump-ipfix-flow SWITCH print flow ipfix of a bridge\n" + " ct-flush-zone SWITCH ZONE flush conntrack entries in ZONE\n" "\nFor OpenFlow switches and controllers:\n" " probe TARGET probe whether TARGET is up\n" " ping TARGET [N] latency of N-byte echos\n" @@ -2606,6 +2607,27 @@ ofctl_dump_ipfix_bridge(struct ovs_cmdl_context *ctx) } static void +ofctl_ct_flush_zone(struct ovs_cmdl_context *ctx) +{ + uint16_t zone_id; + char *error = str_to_u16(ctx->argv[2], "zone_id", &zone_id); + if (error) { + ovs_fatal(0, "%s", error); + } + + struct vconn *vconn; + open_vconn(ctx->argv[1], &vconn); + enum ofp_version version = vconn_get_version(vconn); + + struct ofpbuf *msg = ofpraw_alloc(OFPRAW_NXT_CT_FLUSH_ZONE, version, 0); + struct nx_zone_id *nzi = ofpbuf_put_zeros(msg, sizeof *nzi); + nzi->zone_id = htons(zone_id); + + transact_noreply(vconn, msg); + vconn_close(vconn); +} + +static void ofctl_dump_ipfix_flow(struct ovs_cmdl_context *ctx) { dump_trivial_transaction(ctx->argv[1], OFPRAW_NXST_IPFIX_FLOW_REQUEST); @@ -4488,6 +4510,9 @@ static const struct ovs_cmdl_command all_commands[] = { { "dump-ipfix-flow", "switch", 1, 1, ofctl_dump_ipfix_flow, OVS_RO }, + { "ct-flush-zone", "switch zone", + 2, 2, ofctl_ct_flush_zone, OVS_RO }, + { "ofp-parse", "file", 1, 1, ofctl_ofp_parse, OVS_RW }, { "ofp-parse-pcap", "pcap",
Signed-off-by: Justin Pettit <jpettit@ovn.org> --- NEWS | 1 + debian/changelog | 10 ++++++++++ include/openflow/nicira-ext.h | 9 +++++++++ include/openvswitch/ofp-msgs.h | 4 ++++ lib/dpif.c | 14 ++++++++++++++ lib/dpif.h | 1 + lib/ofp-print.c | 10 ++++++++++ lib/ofp-util.c | 1 + lib/rconn.c | 1 + ofproto/ofproto-dpif.c | 9 +++++++++ ofproto/ofproto-provider.h | 7 +++++++ ofproto/ofproto.c | 23 +++++++++++++++++++++++ tests/ofp-print.at | 10 ++++++++++ tests/ovs-ofctl.at | 13 +++++++++++++ utilities/ovs-ofctl.8.in | 6 ++++++ utilities/ovs-ofctl.c | 25 +++++++++++++++++++++++++ 16 files changed, 144 insertions(+)