| Message ID | 20171108030402.26837-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] ofproto: Delete all groups and meters when (un)configuring a controller. | expand |
On Tue, Nov 07, 2017 at 07:04:02PM -0800, Ben Pfaff wrote: > Open vSwitch has always deleted all flows from the flow table whenever a > controller is configured or whenever all the controllers are unconfigured. > After this commit, OVS additionally deletes all OpenFlow groups and meters. > > Suggested-by: Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com> > Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > v1->v2: Clear the meter table too (thanks Jan). Anyone want to review this? Thanks, Ben.
On 12/22/2017 2:09 PM, Ben Pfaff wrote: > On Tue, Nov 07, 2017 at 07:04:02PM -0800, Ben Pfaff wrote: >> Open vSwitch has always deleted all flows from the flow table whenever a >> controller is configured or whenever all the controllers are unconfigured. >> After this commit, OVS additionally deletes all OpenFlow groups and meters. >> >> Suggested-by: Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com> >> Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> >> Signed-off-by: Ben Pfaff <blp@ovn.org> >> --- >> v1->v2: Clear the meter table too (thanks Jan). > Anyone want to review this? I'm doing some reviews and patch testing today, I'll get to this one. - Greg > > Thanks, > > Ben. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 12/26/2017 9:33 AM, Gregory Rose wrote: > On 12/22/2017 2:09 PM, Ben Pfaff wrote: >> On Tue, Nov 07, 2017 at 07:04:02PM -0800, Ben Pfaff wrote: >>> Open vSwitch has always deleted all flows from the flow table >>> whenever a >>> controller is configured or whenever all the controllers are >>> unconfigured. >>> After this commit, OVS additionally deletes all OpenFlow groups and >>> meters. >>> >>> Suggested-by: Periyasamy Palanisamy >>> <periyasamy.palanisamy@ericsson.com> >>> Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> >>> Signed-off-by: Ben Pfaff <blp@ovn.org> >>> --- >>> v1->v2: Clear the meter table too (thanks Jan). >> Anyone want to review this? I can't seem to find the original patch in my mailbox but I applied it from patchworks and tested it with 'make check' and 'make check-kmod'. The patch itself looks good to me and it passes checkpatch. Tested-by: Greg Rose <gvrose8192@gmail.com> Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Just one small observation below. Otherwise LGTM. I have tested the patch and it worked for all cases. I couldn't test the case that the switch loses connection to a controller in stand-alone fail mode. Acked-by: Jan Scheurich <jan.scheurich@ericsson.com> Tested-by: Jan Scheurich <jan.scheurich@ericsson.com> > -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Wednesday, 08 November, 2017 04:04 > To: dev@openvswitch.org > Cc: Ben Pfaff <blp@ovn.org>; Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com>; Jan Scheurich > <jan.scheurich@ericsson.com> > Subject: [PATCH v2] ofproto: Delete all groups and meters when (un)configuring a controller. > > Open vSwitch has always deleted all flows from the flow table whenever a > controller is configured or whenever all the controllers are unconfigured. > After this commit, OVS additionally deletes all OpenFlow groups and meters. > > Suggested-by: Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com> > Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > v1->v2: Clear the meter table too (thanks Jan). > > AUTHORS.rst | 1 + > NEWS | 3 +++ > ofproto/ofproto.c | 26 +++++++++++++++++------ > tests/ofproto.at | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > vswitchd/vswitch.xml | 15 +++++++------- > 5 files changed, 90 insertions(+), 13 deletions(-) > > diff --git a/AUTHORS.rst b/AUTHORS.rst > index 139e99b330d8..26f81508d3d5 100644 > --- a/AUTHORS.rst > +++ b/AUTHORS.rst > @@ -519,6 +519,7 @@ Pasi Kärkkäinen pasik@iki.fi > Patrik Andersson R patrik.r.andersson@ericsson.com > Paulo Cravero pcravero@as2594.net > Pawan Shukla shuklap@vmware.com > +Periyasamy Palanisamy periyasamy.palanisamy@ericsson.com > Peter Amidon peter@picnicpark.org > Peter Balland peter@nicira.com > Peter Phaal peter.phaal@inmon.com > diff --git a/NEWS b/NEWS > index 047f34b9f402..7ef4d6728d28 100644 > --- a/NEWS > +++ b/NEWS > @@ -1,5 +1,8 @@ > Post-v2.8.0 > -------------------- > + - ovs-vswitchd: > + * Configuring a controller, or unconfiguring all controllers, now deletes > + all groups and meters (as well as all flows). > - OVN: > * The "requested-chassis" option for a logical switch port now accepts a > chassis "hostname" in addition to a chassis "name". > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 82c2bb27d348..e762888b746e 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -251,6 +251,8 @@ static void delete_flows__(struct rule_collection *, > const struct openflow_mod_requester *) > OVS_REQUIRES(ofproto_mutex); > > +static void ofproto_group_delete_all__(struct ofproto *) > + OVS_REQUIRES(ofproto_mutex); > static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id); > static void handle_openflow(struct ofconn *, const struct ofpbuf *); > static enum ofperr ofproto_flow_mod_init(struct ofproto *, > @@ -1566,6 +1568,8 @@ ofproto_flush__(struct ofproto *ofproto) > } > delete_flows__(&rules, OFPRR_DELETE, NULL); > } > + ofproto_group_delete_all__(ofproto); > + meter_delete_all(ofproto); When the ofproto instance is destroyed meter_delete_all() appears to be now invoked twice: first from ofproto_destroy() directly and then a second time from here. It doesn't seem to do any harm, but I guess it would be cleaner to remove the meter_delete_all() call from ofproto_destroy(). It seems that the hmap_destroy(&p->meters) call in ofproto_destroy() should also better be moved to the rcu-deferred ofproto_destroy__() function. > /* XXX: Concurrent handler threads may insert new learned flows based on > * learn actions of the now deleted flows right after we release > * 'ofproto_mutex'. */ > @@ -7348,20 +7352,30 @@ ofproto_group_mod_finish(struct ofproto *ofproto, > * > * This is intended for use within an ofproto provider's 'destruct' > * function. */ > -void > -ofproto_group_delete_all(struct ofproto *ofproto) > - OVS_EXCLUDED(ofproto_mutex) > +static void > +ofproto_group_delete_all__(struct ofproto *ofproto) > + OVS_REQUIRES(ofproto_mutex) > { > struct ofproto_group_mod ogm; > - > ogm.gm.command = OFPGC11_DELETE; > ogm.gm.group_id = OFPG_ALL; > - > - ovs_mutex_lock(&ofproto_mutex); > ogm.version = ofproto->tables_version + 1; > + > ofproto_group_mod_start(ofproto, &ogm); > ofproto_bump_tables_version(ofproto); > ofproto_group_mod_finish(ofproto, &ogm, NULL); > +} > + > +/* Delete all groups from 'ofproto'. > + * > + * This is intended for use within an ofproto provider's 'destruct' > + * function. */ > +void > +ofproto_group_delete_all(struct ofproto *ofproto) > + OVS_EXCLUDED(ofproto_mutex) > +{ > + ovs_mutex_lock(&ofproto_mutex); > + ofproto_group_delete_all__(ofproto); > ovs_mutex_unlock(&ofproto_mutex); > } > > diff --git a/tests/ofproto.at b/tests/ofproto.at > index 31cb5208fc1c..f51fd7e97859 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -6023,3 +6023,61 @@ OVS_VSWITCHD_STOP(["/NXFMFC_INVALID_TLV_FIELD/d > /tun_metadata0/d > /OFPBAC_BAD_SET_LEN/d"]) > AT_CLEANUP > + > +AT_SETUP([ofproto - flush flows, groups, and meters for controller change]) > +AT_KEYWORDS([flow flows group group meter]) > +OVS_VSWITCHD_START > + > +add_flow_group_and_meter () { > + AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=2]) > + AT_CHECK([ovs-ofctl -O OpenFlow11 add-group br0 group_id=1234,type=all,bucket=output:10 > + AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1']) > +]) > +} > + > +verify_added () { > + AT_CHECK([ovs-ofctl --no-stats dump-flows br0], [0], [dnl > + in_port=1 actions=output:2 > +]) > + AT_CHECK([ovs-ofctl -O OpenFlow11 dump-groups br0], [0], [dnl > +OFPST_GROUP_DESC reply (OF1.1) (xid=0x2): > + group_id=1234,type=all,bucket=actions=output:10 > +]) > + AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2): > +meter=1 pktps burst stats bands= > +type=drop rate=1 burst_size=1 > +]) > +} > + > +verify_deleted () { > + AT_CHECK([ovs-ofctl --no-stats dump-flows br0]) > + AT_CHECK([ovs-ofctl -O OpenFlow11 dump-groups br0], [0], [dnl > +OFPST_GROUP_DESC reply (OF1.1) (xid=0x2): > +]) > + AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2): > +]) > +} > + > +# Add flow, group, meter and check that they're there, without a controller. > +add_flow_group_and_meter > +verify_added > + > +# Set up a controller and verify that the flow and group were deleted, > +# then add them back. > +AT_CHECK([ovs-vsctl set-controller br0 'tcp:<invalid>:6653']) > +verify_deleted > +add_flow_group_and_meter > +verify_added > + > +# Change the conroller and verify that the flow and group are still there. > +AT_CHECK([ovs-vsctl set-controller br0 'tcp:<invalid2>:6653']) > +verify_added > + > +# Clear the controller and verify that the flow and group were deleted. > +AT_CHECK([ovs-vsctl del-controller br0]) > +verify_deleted > + > +OVS_VSWITCHD_STOP(["/<invalid/d"]) > +AT_CLEANUP > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index d7f68393b25e..a12ec0f81545 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -751,12 +751,12 @@ > > <p> > If there are primary controllers, removing all of them clears the > - flow table. If there are no primary controllers, adding one also > - clears the flow table. Other changes to the set of controllers, such > - as adding or removing a service controller, adding another primary > - controller to supplement an existing primary controller, or removing > - only one of two primary controllers, have no effect on the flow > - table. > + OpenFlow flow tables, group table, and meter table. If there are no > + primary controllers, adding one also clears these tables. Other > + changes to the set of controllers, such as adding or removing a > + service controller, adding another primary controller to supplement > + an existing primary controller, or removing only one of two primary > + controllers, have no effect on these tables. > </p> > </column> > > @@ -806,7 +806,8 @@ > configured controllers can be contacted.</p> > <p> > Changing <ref column="fail_mode"/> when no primary controllers are > - configured clears the flow table. > + configured clears the OpenFlow flow tables, group table, and meter > + table. > </p> > </column> > > -- > 2.10.2
Thanks for the review. I agree with your comments. I fixed them and applied this to master. On Fri, Jan 05, 2018 at 11:21:55AM +0000, Jan Scheurich wrote: > Just one small observation below. Otherwise LGTM. > > I have tested the patch and it worked for all cases. I couldn't test the case that the switch loses connection to a controller in stand-alone fail mode. > > Acked-by: Jan Scheurich <jan.scheurich@ericsson.com> > Tested-by: Jan Scheurich <jan.scheurich@ericsson.com> > > > -----Original Message----- > > From: Ben Pfaff [mailto:blp@ovn.org] > > Sent: Wednesday, 08 November, 2017 04:04 > > To: dev@openvswitch.org > > Cc: Ben Pfaff <blp@ovn.org>; Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com>; Jan Scheurich > > <jan.scheurich@ericsson.com> > > Subject: [PATCH v2] ofproto: Delete all groups and meters when (un)configuring a controller. > > > > Open vSwitch has always deleted all flows from the flow table whenever a > > controller is configured or whenever all the controllers are unconfigured. > > After this commit, OVS additionally deletes all OpenFlow groups and meters. > > > > Suggested-by: Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com> > > Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > v1->v2: Clear the meter table too (thanks Jan). > > > > AUTHORS.rst | 1 + > > NEWS | 3 +++ > > ofproto/ofproto.c | 26 +++++++++++++++++------ > > tests/ofproto.at | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > vswitchd/vswitch.xml | 15 +++++++------- > > 5 files changed, 90 insertions(+), 13 deletions(-) > > > > diff --git a/AUTHORS.rst b/AUTHORS.rst > > index 139e99b330d8..26f81508d3d5 100644 > > --- a/AUTHORS.rst > > +++ b/AUTHORS.rst > > @@ -519,6 +519,7 @@ Pasi Kärkkäinen pasik@iki.fi > > Patrik Andersson R patrik.r.andersson@ericsson.com > > Paulo Cravero pcravero@as2594.net > > Pawan Shukla shuklap@vmware.com > > +Periyasamy Palanisamy periyasamy.palanisamy@ericsson.com > > Peter Amidon peter@picnicpark.org > > Peter Balland peter@nicira.com > > Peter Phaal peter.phaal@inmon.com > > diff --git a/NEWS b/NEWS > > index 047f34b9f402..7ef4d6728d28 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -1,5 +1,8 @@ > > Post-v2.8.0 > > -------------------- > > + - ovs-vswitchd: > > + * Configuring a controller, or unconfiguring all controllers, now deletes > > + all groups and meters (as well as all flows). > > - OVN: > > * The "requested-chassis" option for a logical switch port now accepts a > > chassis "hostname" in addition to a chassis "name". > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > > index 82c2bb27d348..e762888b746e 100644 > > --- a/ofproto/ofproto.c > > +++ b/ofproto/ofproto.c > > @@ -251,6 +251,8 @@ static void delete_flows__(struct rule_collection *, > > const struct openflow_mod_requester *) > > OVS_REQUIRES(ofproto_mutex); > > > > +static void ofproto_group_delete_all__(struct ofproto *) > > + OVS_REQUIRES(ofproto_mutex); > > static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id); > > static void handle_openflow(struct ofconn *, const struct ofpbuf *); > > static enum ofperr ofproto_flow_mod_init(struct ofproto *, > > @@ -1566,6 +1568,8 @@ ofproto_flush__(struct ofproto *ofproto) > > } > > delete_flows__(&rules, OFPRR_DELETE, NULL); > > } > > + ofproto_group_delete_all__(ofproto); > > + meter_delete_all(ofproto); > > When the ofproto instance is destroyed meter_delete_all() appears to be now invoked twice: > first from ofproto_destroy() directly and then a second time from here. It doesn't seem to do any harm, > but I guess it would be cleaner to remove the meter_delete_all() call from ofproto_destroy(). > > It seems that the hmap_destroy(&p->meters) call in ofproto_destroy() should also better be moved to the rcu-deferred ofproto_destroy__() function. > > > /* XXX: Concurrent handler threads may insert new learned flows based on > > * learn actions of the now deleted flows right after we release > > * 'ofproto_mutex'. */ > > @@ -7348,20 +7352,30 @@ ofproto_group_mod_finish(struct ofproto *ofproto, > > * > > * This is intended for use within an ofproto provider's 'destruct' > > * function. */ > > -void > > -ofproto_group_delete_all(struct ofproto *ofproto) > > - OVS_EXCLUDED(ofproto_mutex) > > +static void > > +ofproto_group_delete_all__(struct ofproto *ofproto) > > + OVS_REQUIRES(ofproto_mutex) > > { > > struct ofproto_group_mod ogm; > > - > > ogm.gm.command = OFPGC11_DELETE; > > ogm.gm.group_id = OFPG_ALL; > > - > > - ovs_mutex_lock(&ofproto_mutex); > > ogm.version = ofproto->tables_version + 1; > > + > > ofproto_group_mod_start(ofproto, &ogm); > > ofproto_bump_tables_version(ofproto); > > ofproto_group_mod_finish(ofproto, &ogm, NULL); > > +} > > + > > +/* Delete all groups from 'ofproto'. > > + * > > + * This is intended for use within an ofproto provider's 'destruct' > > + * function. */ > > +void > > +ofproto_group_delete_all(struct ofproto *ofproto) > > + OVS_EXCLUDED(ofproto_mutex) > > +{ > > + ovs_mutex_lock(&ofproto_mutex); > > + ofproto_group_delete_all__(ofproto); > > ovs_mutex_unlock(&ofproto_mutex); > > } > > > > diff --git a/tests/ofproto.at b/tests/ofproto.at > > index 31cb5208fc1c..f51fd7e97859 100644 > > --- a/tests/ofproto.at > > +++ b/tests/ofproto.at > > @@ -6023,3 +6023,61 @@ OVS_VSWITCHD_STOP(["/NXFMFC_INVALID_TLV_FIELD/d > > /tun_metadata0/d > > /OFPBAC_BAD_SET_LEN/d"]) > > AT_CLEANUP > > + > > +AT_SETUP([ofproto - flush flows, groups, and meters for controller change]) > > +AT_KEYWORDS([flow flows group group meter]) > > +OVS_VSWITCHD_START > > + > > +add_flow_group_and_meter () { > > + AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=2]) > > + AT_CHECK([ovs-ofctl -O OpenFlow11 add-group br0 group_id=1234,type=all,bucket=output:10 > > + AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1']) > > +]) > > +} > > + > > +verify_added () { > > + AT_CHECK([ovs-ofctl --no-stats dump-flows br0], [0], [dnl > > + in_port=1 actions=output:2 > > +]) > > + AT_CHECK([ovs-ofctl -O OpenFlow11 dump-groups br0], [0], [dnl > > +OFPST_GROUP_DESC reply (OF1.1) (xid=0x2): > > + group_id=1234,type=all,bucket=actions=output:10 > > +]) > > + AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl > > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2): > > +meter=1 pktps burst stats bands= > > +type=drop rate=1 burst_size=1 > > +]) > > +} > > + > > +verify_deleted () { > > + AT_CHECK([ovs-ofctl --no-stats dump-flows br0]) > > + AT_CHECK([ovs-ofctl -O OpenFlow11 dump-groups br0], [0], [dnl > > +OFPST_GROUP_DESC reply (OF1.1) (xid=0x2): > > +]) > > + AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl > > +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2): > > +]) > > +} > > + > > +# Add flow, group, meter and check that they're there, without a controller. > > +add_flow_group_and_meter > > +verify_added > > + > > +# Set up a controller and verify that the flow and group were deleted, > > +# then add them back. > > +AT_CHECK([ovs-vsctl set-controller br0 'tcp:<invalid>:6653']) > > +verify_deleted > > +add_flow_group_and_meter > > +verify_added > > + > > +# Change the conroller and verify that the flow and group are still there. > > +AT_CHECK([ovs-vsctl set-controller br0 'tcp:<invalid2>:6653']) > > +verify_added > > + > > +# Clear the controller and verify that the flow and group were deleted. > > +AT_CHECK([ovs-vsctl del-controller br0]) > > +verify_deleted > > + > > +OVS_VSWITCHD_STOP(["/<invalid/d"]) > > +AT_CLEANUP > > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > > index d7f68393b25e..a12ec0f81545 100644 > > --- a/vswitchd/vswitch.xml > > +++ b/vswitchd/vswitch.xml > > @@ -751,12 +751,12 @@ > > > > <p> > > If there are primary controllers, removing all of them clears the > > - flow table. If there are no primary controllers, adding one also > > - clears the flow table. Other changes to the set of controllers, such > > - as adding or removing a service controller, adding another primary > > - controller to supplement an existing primary controller, or removing > > - only one of two primary controllers, have no effect on the flow > > - table. > > + OpenFlow flow tables, group table, and meter table. If there are no > > + primary controllers, adding one also clears these tables. Other > > + changes to the set of controllers, such as adding or removing a > > + service controller, adding another primary controller to supplement > > + an existing primary controller, or removing only one of two primary > > + controllers, have no effect on these tables. > > </p> > > </column> > > > > @@ -806,7 +806,8 @@ > > configured controllers can be contacted.</p> > > <p> > > Changing <ref column="fail_mode"/> when no primary controllers are > > - configured clears the flow table. > > + configured clears the OpenFlow flow tables, group table, and meter > > + table. > > </p> > > </column> > > > > -- > > 2.10.2 >
On Tue, Dec 26, 2017 at 11:12:02AM -0800, Gregory Rose wrote: > On 12/26/2017 9:33 AM, Gregory Rose wrote: > >On 12/22/2017 2:09 PM, Ben Pfaff wrote: > >>On Tue, Nov 07, 2017 at 07:04:02PM -0800, Ben Pfaff wrote: > >>>Open vSwitch has always deleted all flows from the flow table whenever > >>>a > >>>controller is configured or whenever all the controllers are > >>>unconfigured. > >>>After this commit, OVS additionally deletes all OpenFlow groups and > >>>meters. > >>> > >>>Suggested-by: Periyasamy Palanisamy > >>><periyasamy.palanisamy@ericsson.com> > >>>Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> > >>>Signed-off-by: Ben Pfaff <blp@ovn.org> > >>>--- > >>>v1->v2: Clear the meter table too (thanks Jan). > >>Anyone want to review this? > > I can't seem to find the original patch in my mailbox but I applied it from > patchworks and tested it > with 'make check' and 'make check-kmod'. The patch itself looks good to me > and it passes checkpatch. > > Tested-by: Greg Rose <gvrose8192@gmail.com> > Reviewed-by: Greg Rose <gvrose8192@gmail.com> > Thanks for the review. I applied this to master.
diff --git a/AUTHORS.rst b/AUTHORS.rst index 139e99b330d8..26f81508d3d5 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -519,6 +519,7 @@ Pasi Kärkkäinen pasik@iki.fi Patrik Andersson R patrik.r.andersson@ericsson.com Paulo Cravero pcravero@as2594.net Pawan Shukla shuklap@vmware.com +Periyasamy Palanisamy periyasamy.palanisamy@ericsson.com Peter Amidon peter@picnicpark.org Peter Balland peter@nicira.com Peter Phaal peter.phaal@inmon.com diff --git a/NEWS b/NEWS index 047f34b9f402..7ef4d6728d28 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,8 @@ Post-v2.8.0 -------------------- + - ovs-vswitchd: + * Configuring a controller, or unconfiguring all controllers, now deletes + all groups and meters (as well as all flows). - OVN: * The "requested-chassis" option for a logical switch port now accepts a chassis "hostname" in addition to a chassis "name". diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 82c2bb27d348..e762888b746e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -251,6 +251,8 @@ static void delete_flows__(struct rule_collection *, const struct openflow_mod_requester *) OVS_REQUIRES(ofproto_mutex); +static void ofproto_group_delete_all__(struct ofproto *) + OVS_REQUIRES(ofproto_mutex); static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id); static void handle_openflow(struct ofconn *, const struct ofpbuf *); static enum ofperr ofproto_flow_mod_init(struct ofproto *, @@ -1566,6 +1568,8 @@ ofproto_flush__(struct ofproto *ofproto) } delete_flows__(&rules, OFPRR_DELETE, NULL); } + ofproto_group_delete_all__(ofproto); + meter_delete_all(ofproto); /* XXX: Concurrent handler threads may insert new learned flows based on * learn actions of the now deleted flows right after we release * 'ofproto_mutex'. */ @@ -7348,20 +7352,30 @@ ofproto_group_mod_finish(struct ofproto *ofproto, * * This is intended for use within an ofproto provider's 'destruct' * function. */ -void -ofproto_group_delete_all(struct ofproto *ofproto) - OVS_EXCLUDED(ofproto_mutex) +static void +ofproto_group_delete_all__(struct ofproto *ofproto) + OVS_REQUIRES(ofproto_mutex) { struct ofproto_group_mod ogm; - ogm.gm.command = OFPGC11_DELETE; ogm.gm.group_id = OFPG_ALL; - - ovs_mutex_lock(&ofproto_mutex); ogm.version = ofproto->tables_version + 1; + ofproto_group_mod_start(ofproto, &ogm); ofproto_bump_tables_version(ofproto); ofproto_group_mod_finish(ofproto, &ogm, NULL); +} + +/* Delete all groups from 'ofproto'. + * + * This is intended for use within an ofproto provider's 'destruct' + * function. */ +void +ofproto_group_delete_all(struct ofproto *ofproto) + OVS_EXCLUDED(ofproto_mutex) +{ + ovs_mutex_lock(&ofproto_mutex); + ofproto_group_delete_all__(ofproto); ovs_mutex_unlock(&ofproto_mutex); } diff --git a/tests/ofproto.at b/tests/ofproto.at index 31cb5208fc1c..f51fd7e97859 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -6023,3 +6023,61 @@ OVS_VSWITCHD_STOP(["/NXFMFC_INVALID_TLV_FIELD/d /tun_metadata0/d /OFPBAC_BAD_SET_LEN/d"]) AT_CLEANUP + +AT_SETUP([ofproto - flush flows, groups, and meters for controller change]) +AT_KEYWORDS([flow flows group group meter]) +OVS_VSWITCHD_START + +add_flow_group_and_meter () { + AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=2]) + AT_CHECK([ovs-ofctl -O OpenFlow11 add-group br0 group_id=1234,type=all,bucket=output:10 + AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps burst stats bands=type=drop rate=1 burst_size=1']) +]) +} + +verify_added () { + AT_CHECK([ovs-ofctl --no-stats dump-flows br0], [0], [dnl + in_port=1 actions=output:2 +]) + AT_CHECK([ovs-ofctl -O OpenFlow11 dump-groups br0], [0], [dnl +OFPST_GROUP_DESC reply (OF1.1) (xid=0x2): + group_id=1234,type=all,bucket=actions=output:10 +]) + AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2): +meter=1 pktps burst stats bands= +type=drop rate=1 burst_size=1 +]) +} + +verify_deleted () { + AT_CHECK([ovs-ofctl --no-stats dump-flows br0]) + AT_CHECK([ovs-ofctl -O OpenFlow11 dump-groups br0], [0], [dnl +OFPST_GROUP_DESC reply (OF1.1) (xid=0x2): +]) + AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0], [0], [dnl +OFPST_METER_CONFIG reply (OF1.3) (xid=0x2): +]) +} + +# Add flow, group, meter and check that they're there, without a controller. +add_flow_group_and_meter +verify_added + +# Set up a controller and verify that the flow and group were deleted, +# then add them back. +AT_CHECK([ovs-vsctl set-controller br0 'tcp:<invalid>:6653']) +verify_deleted +add_flow_group_and_meter +verify_added + +# Change the conroller and verify that the flow and group are still there. +AT_CHECK([ovs-vsctl set-controller br0 'tcp:<invalid2>:6653']) +verify_added + +# Clear the controller and verify that the flow and group were deleted. +AT_CHECK([ovs-vsctl del-controller br0]) +verify_deleted + +OVS_VSWITCHD_STOP(["/<invalid/d"]) +AT_CLEANUP diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index d7f68393b25e..a12ec0f81545 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -751,12 +751,12 @@ <p> If there are primary controllers, removing all of them clears the - flow table. If there are no primary controllers, adding one also - clears the flow table. Other changes to the set of controllers, such - as adding or removing a service controller, adding another primary - controller to supplement an existing primary controller, or removing - only one of two primary controllers, have no effect on the flow - table. + OpenFlow flow tables, group table, and meter table. If there are no + primary controllers, adding one also clears these tables. Other + changes to the set of controllers, such as adding or removing a + service controller, adding another primary controller to supplement + an existing primary controller, or removing only one of two primary + controllers, have no effect on these tables. </p> </column> @@ -806,7 +806,8 @@ configured controllers can be contacted.</p> <p> Changing <ref column="fail_mode"/> when no primary controllers are - configured clears the flow table. + configured clears the OpenFlow flow tables, group table, and meter + table. </p> </column>
Open vSwitch has always deleted all flows from the flow table whenever a controller is configured or whenever all the controllers are unconfigured. After this commit, OVS additionally deletes all OpenFlow groups and meters. Suggested-by: Periyasamy Palanisamy <periyasamy.palanisamy@ericsson.com> Suggested-by: Jan Scheurich <jan.scheurich@ericsson.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- v1->v2: Clear the meter table too (thanks Jan). AUTHORS.rst | 1 + NEWS | 3 +++ ofproto/ofproto.c | 26 +++++++++++++++++------ tests/ofproto.at | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++ vswitchd/vswitch.xml | 15 +++++++------- 5 files changed, 90 insertions(+), 13 deletions(-)