[ovs-dev] ofproto: Report only un-deleted groups in group stats replies.

Message ID 20170419182915.3326-1-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff April 19, 2017, 6:29 p.m.
Deleted groups hang around in the group table until the next grace period,
so it's important for the group stats code to pretend that they're gone
until they really get deleted.

Reported-by: "Timothy M. Redaelli" <tredaelli@redhat.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331117.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Greg Rose April 19, 2017, 6:36 p.m. | #1
On Wed, 2017-04-19 at 11:29 -0700, Ben Pfaff wrote:
> Deleted groups hang around in the group table until the next grace period,
> so it's important for the group stats code to pretend that they're gone
> until they really get deleted.
> 
> Reported-by: "Timothy M. Redaelli" <tredaelli@redhat.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331117.html
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>  ofproto/ofproto.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7440d5b52092..4d3d46c8ba45 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6641,7 +6641,10 @@ handle_group_request(struct ofconn *ofconn,
>      ovs_mutex_lock(&ofproto_mutex);
>      if (group_id == OFPG_ALL) {
>          CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) {
> -            cb(group, &replies);
> +            if (versions_visible_in_version(&group->versions,
> +                                            OVS_VERSION_MAX)) {
> +                cb(group, &replies);
> +            }
>          }
>      } else {
>          group = ofproto_group_lookup__(ofproto, group_id, OVS_VERSION_MAX);

Looks good to me.

Acked-by: Greg Rose <gvrose8192@gmail.com>
Jarno Rajahalme April 19, 2017, 7:55 p.m. | #2
This was an oversight from when we groups were versioned for OF bundle support. Looks good to me, too.

Acked-by: Jarno Rajahalme <jarno@ovn.org>

> On Apr 19, 2017, at 11:29 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Deleted groups hang around in the group table until the next grace period,
> so it's important for the group stats code to pretend that they're gone
> until they really get deleted.
> 
> Reported-by: "Timothy M. Redaelli" <tredaelli@redhat.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331117.html
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
> ofproto/ofproto.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7440d5b52092..4d3d46c8ba45 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6641,7 +6641,10 @@ handle_group_request(struct ofconn *ofconn,
>     ovs_mutex_lock(&ofproto_mutex);
>     if (group_id == OFPG_ALL) {
>         CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) {
> -            cb(group, &replies);
> +            if (versions_visible_in_version(&group->versions,
> +                                            OVS_VERSION_MAX)) {
> +                cb(group, &replies);
> +            }
>         }
>     } else {
>         group = ofproto_group_lookup__(ofproto, group_id, OVS_VERSION_MAX);
> -- 
> 2.10.2
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff April 19, 2017, 9:13 p.m. | #3
Thanks,

I'm going to give the reporter a day or so to verify that his test now
passes, just in case, but in case I don't hear from him I'll apply it at
that point.

On Wed, Apr 19, 2017 at 12:55:37PM -0700, Jarno Rajahalme wrote:
> This was an oversight from when we groups were versioned for OF bundle support. Looks good to me, too.
> 
> Acked-by: Jarno Rajahalme <jarno@ovn.org>
> 
> > On Apr 19, 2017, at 11:29 AM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > Deleted groups hang around in the group table until the next grace period,
> > so it's important for the group stats code to pretend that they're gone
> > until they really get deleted.
> > 
> > Reported-by: "Timothy M. Redaelli" <tredaelli@redhat.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331117.html
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> > ofproto/ofproto.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 7440d5b52092..4d3d46c8ba45 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -6641,7 +6641,10 @@ handle_group_request(struct ofconn *ofconn,
> >     ovs_mutex_lock(&ofproto_mutex);
> >     if (group_id == OFPG_ALL) {
> >         CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) {
> > -            cb(group, &replies);
> > +            if (versions_visible_in_version(&group->versions,
> > +                                            OVS_VERSION_MAX)) {
> > +                cb(group, &replies);
> > +            }
> >         }
> >     } else {
> >         group = ofproto_group_lookup__(ofproto, group_id, OVS_VERSION_MAX);
> > -- 
> > 2.10.2
> > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff April 19, 2017, 9:19 p.m. | #4
Thanks,

I'm going to give the reporter a day or so to verify that his test now
passes, just in case, but in case I don't hear from him I'll apply it at
that point.

On Wed, Apr 19, 2017 at 11:36:33AM -0700, Greg Rose wrote:
> On Wed, 2017-04-19 at 11:29 -0700, Ben Pfaff wrote:
> > Deleted groups hang around in the group table until the next grace period,
> > so it's important for the group stats code to pretend that they're gone
> > until they really get deleted.
> > 
> > Reported-by: "Timothy M. Redaelli" <tredaelli@redhat.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-April/331117.html
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> >  ofproto/ofproto.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 7440d5b52092..4d3d46c8ba45 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -6641,7 +6641,10 @@ handle_group_request(struct ofconn *ofconn,
> >      ovs_mutex_lock(&ofproto_mutex);
> >      if (group_id == OFPG_ALL) {
> >          CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) {
> > -            cb(group, &replies);
> > +            if (versions_visible_in_version(&group->versions,
> > +                                            OVS_VERSION_MAX)) {
> > +                cb(group, &replies);
> > +            }
> >          }
> >      } else {
> >          group = ofproto_group_lookup__(ofproto, group_id, OVS_VERSION_MAX);
> 
> Looks good to me.
> 
> Acked-by: Greg Rose <gvrose8192@gmail.com>
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Timothy M. Redaelli April 20, 2017, 9:19 a.m. | #5
On 04/19/2017 08:29 PM, Ben Pfaff wrote:
> Deleted groups hang around in the group table until the next grace period,
> so it's important for the group stats code to pretend that they're gone
> until they really get deleted.

[...]
Thank you for the patch, I tested it in all the scenarios and it works.

Tested-by: Timothy Redaelli <tredaelli@redhat.com>
Ben Pfaff April 21, 2017, 9:10 p.m. | #6
On Thu, Apr 20, 2017 at 11:19:07AM +0200, Timothy M. Redaelli wrote:
> On 04/19/2017 08:29 PM, Ben Pfaff wrote:
> > Deleted groups hang around in the group table until the next grace period,
> > so it's important for the group stats code to pretend that they're gone
> > until they really get deleted.
> 
> [...]
> Thank you for the patch, I tested it in all the scenarios and it works.
> 
> Tested-by: Timothy Redaelli <tredaelli@redhat.com>

Thanks, I applied this to master, branch-2.7, and branch-2.6.  (Earlier
versions didn't have this problem.)

Patch

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 7440d5b52092..4d3d46c8ba45 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6641,7 +6641,10 @@  handle_group_request(struct ofconn *ofconn,
     ovs_mutex_lock(&ofproto_mutex);
     if (group_id == OFPG_ALL) {
         CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) {
-            cb(group, &replies);
+            if (versions_visible_in_version(&group->versions,
+                                            OVS_VERSION_MAX)) {
+                cb(group, &replies);
+            }
         }
     } else {
         group = ofproto_group_lookup__(ofproto, group_id, OVS_VERSION_MAX);