[ovs-dev,v2] ofproto: Delete all groups and meters when (un)configuring a controller.

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.
Related show

Commit Message

Ben Pfaff Nov. 8, 2017, 3:04 a.m.
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(-)

Comments

Ben Pfaff Dec. 22, 2017, 10:09 p.m. | #1
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.
Gregory Rose Dec. 26, 2017, 5:33 p.m. | #2
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
Gregory Rose Dec. 26, 2017, 7:12 p.m. | #3
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>
Jan Scheurich Jan. 5, 2018, 11:21 a.m. | #4
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
Ben Pfaff Jan. 8, 2018, 9:17 p.m. | #5
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
>
Ben Pfaff Jan. 8, 2018, 9:17 p.m. | #6
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.

Patch

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>