| Message ID | 20170419182915.3326-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
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>
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
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 >
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
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>
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.)
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);
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(-)