Re: [ovs-dev] [PATCH ovn] extend-table: Fix table ID double allocation after OVS restart.
On Thu, Jul 28, 2022 at 1:34 PM Numan Siddique wrote:
>
> On Wed, Jul 27, 2022 at 6:18 PM Han Zhou wrote:
> >
> > There were problems observed occasionally after OVS restart, the
> > OVS flow bundle installation from ovn-controller was failed because of
> > "GROUP_EXISTS" error, which end up with missing flows/groups/meters
> > in OVS until ovn-controller is restarted.
> >
>
> Hi Han,
>
> Can you please check this bugzilla and see if its the same issue ?
>
Yes.
> https://bugzilla.redhat.com/show_bug.cgi?id=2112111
>
> Looks to me it is. If so, can you please also add - "Reported-at" tag
> referencing this BZ (for our tracking purpose).
Ack.
>
> Please see a few small nits.
>
> > Example error logs in OVS:
> > 2022-07-08T01:38:22.837Z|00676|connmgr|INFO|br-int<->unix#0: sending
OFPGMFC_GROUP_EXISTS error reply to OFPT_BUNDLE_ADD_MESSAGE message
> > 2022-07-08T01:38:22.913Z|00677|connmgr|INFO|br-int<->unix#0: sending
OFPBFC_MSG_FAILED error reply to OFPT_BUNDLE_CONTROL message
> >
> > The root cause is that with ovn-ofctrl-wait-before-clear set, ofctrl
> > module would call ovn_extend_table_clear() to clear the existing group
> > table AFTER ovn-controll finished computing the desired
>
> s/ovn-controll/ovn-controller
Ack.
>
> > flows/groups/meters in the state S_CLEAR. However, the function
> > ovn_extend_table_clear() clears the bitmap of the group IDs, while the
> > IDs are still being used by desired group table. This is not a problem
>
> s/by desired group/by the group
I meant to emphasize it is the "desired" group table that still uses the
bitmap although the "existing" group table is cleared.
I rephrased it to avoid the confusion.
>
> > if a recompute happens soon, the desired group table will be cleared
> > first and IDs will be reallocated and the bitmap will reflect the actual
> > allocations. However, if there are any group creation changes (related
> > to LB, ECMP, etc.) happen before the recompute, new IDs may be allocated
> > to be conflict with existing IDs because the cleared bitmap status
> > doesn't reflect the real IDs being used. The conflict IDs finally causes
> > the "GROUP_EXIST" error replied by OVS when ovn-controller tries to
> > install the desired groups to OVS. Even worse, because the group
> > modifications are now wrapped in a bundle with flow modifications, it
> > would end up with not only missing groups but also missing flows.
> >
> > Both desired table and existing table share the same bitmap, which is to
> > avoid reallocating an ID that is still exists in OVS, but the current
>
> s/that is still/that still
>
Ack.
> > logic seems to have an assumption that the "existing" table entries are
> > deleted always AFTER the "desired" entries. This assumption is not true
> > after the introduction of ovn-ofctrl-wait-before-clear feature.
> >
> > The fix here is to introduce a reference between the desired and
> > existing entries, so that when deleting an entry in either of the tables
> > it knows if the ID is still in use by its peer and decide if it is the
> > right time to clear the bit from the bitmap, without depending on the
> > order of deletion.
> >
> > Fixes: 896adfd2d8b3 ("ofctrl: Support ovn-ofctrl-wait-before-clear to
reduce down time during upgrade.")
> > Signed-off-by: Han Zhou
> > ---
> > lib/extend-table.c | 67 +
> > lib/extend-table.h | 17 ---
> > tests/ovn-controller.at | 26 +++-
> > 3 files changed, 73 insertions(+), 37 deletions(-)
> >
> > diff --git a/lib/extend-table.c b/lib/extend-table.c
> > index 4c3c4fac2..453b4468a 100644
> > --- a/lib/extend-table.c
> > +++ b/lib/extend-table.c
> > @@ -40,13 +40,17 @@ ovn_extend_table_init(struct ovn_extend_table
*table)
> > }
> >
> > static struct ovn_extend_table_info *
> > -ovn_extend_table_info_alloc(const char *name, uint32_t id, bool
is_new_id,
> > +ovn_extend_table_info_alloc(const char *name, uint32_t id,
> > +struct ovn_extend_table_info *peer,
> > uint32_t hash)
> > {
> > struct ovn_extend_table_info *e = xmalloc(sizeof *e);
> > e->name = xstrdup(name);
> > e->table_id = id;
> > -e->new_table_id = is_new_id;
> > +e->peer = peer;
> > +if (peer) {
> > +peer->peer = e;
> > +}
> > e->hmap_node.hash = hash;
> > hmap_init(&e->references);
> > return e;
> > @@ -184,9 +188,10 @@ ovn_extend_table_clear(struct ovn_extend_table
*table, bool existing)
> > /* Clear the target table. */
> > HMAP_FOR_EACH_SAFE (g, hmap_node, target) {
> > hmap_remove(target, &g->hmap_node);
> > -/* Don't unset bitmap for desired group_info if the group_id
> > - * was not freshly reserved. */
> > -if (existing || g->new_table_id) {
> > +if (g->peer) {
> > +g->peer->peer = NULL;
> > +} else {
> > +/* Don't unset bitmap if the peer table is still using it.
*/
>
> This
Re: [ovs-dev] [PATCH ovn] extend-table: Fix table ID double allocation after OVS restart.
On Wed, Jul 27, 2022 at 6:18 PM Han Zhou wrote:
>
> There were problems observed occasionally after OVS restart, the
> OVS flow bundle installation from ovn-controller was failed because of
> "GROUP_EXISTS" error, which end up with missing flows/groups/meters
> in OVS until ovn-controller is restarted.
>
Hi Han,
Can you please check this bugzilla and see if its the same issue ?
https://bugzilla.redhat.com/show_bug.cgi?id=2112111
Looks to me it is. If so, can you please also add - "Reported-at" tag
referencing this BZ (for our tracking purpose).
Please see a few small nits.
> Example error logs in OVS:
> 2022-07-08T01:38:22.837Z|00676|connmgr|INFO|br-int<->unix#0: sending
> OFPGMFC_GROUP_EXISTS error reply to OFPT_BUNDLE_ADD_MESSAGE message
> 2022-07-08T01:38:22.913Z|00677|connmgr|INFO|br-int<->unix#0: sending
> OFPBFC_MSG_FAILED error reply to OFPT_BUNDLE_CONTROL message
>
> The root cause is that with ovn-ofctrl-wait-before-clear set, ofctrl
> module would call ovn_extend_table_clear() to clear the existing group
> table AFTER ovn-controll finished computing the desired
s/ovn-controll/ovn-controller
> flows/groups/meters in the state S_CLEAR. However, the function
> ovn_extend_table_clear() clears the bitmap of the group IDs, while the
> IDs are still being used by desired group table. This is not a problem
s/by desired group/by the group
> if a recompute happens soon, the desired group table will be cleared
> first and IDs will be reallocated and the bitmap will reflect the actual
> allocations. However, if there are any group creation changes (related
> to LB, ECMP, etc.) happen before the recompute, new IDs may be allocated
> to be conflict with existing IDs because the cleared bitmap status
> doesn't reflect the real IDs being used. The conflict IDs finally causes
> the "GROUP_EXIST" error replied by OVS when ovn-controller tries to
> install the desired groups to OVS. Even worse, because the group
> modifications are now wrapped in a bundle with flow modifications, it
> would end up with not only missing groups but also missing flows.
>
> Both desired table and existing table share the same bitmap, which is to
> avoid reallocating an ID that is still exists in OVS, but the current
s/that is still/that still
> logic seems to have an assumption that the "existing" table entries are
> deleted always AFTER the "desired" entries. This assumption is not true
> after the introduction of ovn-ofctrl-wait-before-clear feature.
>
> The fix here is to introduce a reference between the desired and
> existing entries, so that when deleting an entry in either of the tables
> it knows if the ID is still in use by its peer and decide if it is the
> right time to clear the bit from the bitmap, without depending on the
> order of deletion.
>
> Fixes: 896adfd2d8b3 ("ofctrl: Support ovn-ofctrl-wait-before-clear to reduce
> down time during upgrade.")
> Signed-off-by: Han Zhou
> ---
> lib/extend-table.c | 67 +
> lib/extend-table.h | 17 ---
> tests/ovn-controller.at | 26 +++-
> 3 files changed, 73 insertions(+), 37 deletions(-)
>
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index 4c3c4fac2..453b4468a 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -40,13 +40,17 @@ ovn_extend_table_init(struct ovn_extend_table *table)
> }
>
> static struct ovn_extend_table_info *
> -ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id,
> +ovn_extend_table_info_alloc(const char *name, uint32_t id,
> +struct ovn_extend_table_info *peer,
> uint32_t hash)
> {
> struct ovn_extend_table_info *e = xmalloc(sizeof *e);
> e->name = xstrdup(name);
> e->table_id = id;
> -e->new_table_id = is_new_id;
> +e->peer = peer;
> +if (peer) {
> +peer->peer = e;
> +}
> e->hmap_node.hash = hash;
> hmap_init(&e->references);
> return e;
> @@ -184,9 +188,10 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
> bool existing)
> /* Clear the target table. */
> HMAP_FOR_EACH_SAFE (g, hmap_node, target) {
> hmap_remove(target, &g->hmap_node);
> -/* Don't unset bitmap for desired group_info if the group_id
> - * was not freshly reserved. */
> -if (existing || g->new_table_id) {
> +if (g->peer) {
> +g->peer->peer = NULL;
> +} else {
> +/* Don't unset bitmap if the peer table is still using it. */
This comment is confusing to me. The comment says don't unset bitmap
but the function bitmap_set0() unsets the bit right ?
Maybe the comment should be in the "if" condition ?
With these small comments addressed
Acked-by: Numan Siddique
Numan
> bitmap_set0(table->table_ids, g->table_id);
> }
> ovn_extend_table_info_destroy(g);
> @@ -209,11 +214,15 @@ void
> ovn_extend_table_remove_existing(struct ovn
[ovs-dev] [PATCH ovn] extend-table: Fix table ID double allocation after OVS restart.
There were problems observed occasionally after OVS restart, the
OVS flow bundle installation from ovn-controller was failed because of
"GROUP_EXISTS" error, which end up with missing flows/groups/meters
in OVS until ovn-controller is restarted.
Example error logs in OVS:
2022-07-08T01:38:22.837Z|00676|connmgr|INFO|br-int<->unix#0: sending
OFPGMFC_GROUP_EXISTS error reply to OFPT_BUNDLE_ADD_MESSAGE message
2022-07-08T01:38:22.913Z|00677|connmgr|INFO|br-int<->unix#0: sending
OFPBFC_MSG_FAILED error reply to OFPT_BUNDLE_CONTROL message
The root cause is that with ovn-ofctrl-wait-before-clear set, ofctrl
module would call ovn_extend_table_clear() to clear the existing group
table AFTER ovn-controll finished computing the desired
flows/groups/meters in the state S_CLEAR. However, the function
ovn_extend_table_clear() clears the bitmap of the group IDs, while the
IDs are still being used by desired group table. This is not a problem
if a recompute happens soon, the desired group table will be cleared
first and IDs will be reallocated and the bitmap will reflect the actual
allocations. However, if there are any group creation changes (related
to LB, ECMP, etc.) happen before the recompute, new IDs may be allocated
to be conflict with existing IDs because the cleared bitmap status
doesn't reflect the real IDs being used. The conflict IDs finally causes
the "GROUP_EXIST" error replied by OVS when ovn-controller tries to
install the desired groups to OVS. Even worse, because the group
modifications are now wrapped in a bundle with flow modifications, it
would end up with not only missing groups but also missing flows.
Both desired table and existing table share the same bitmap, which is to
avoid reallocating an ID that is still exists in OVS, but the current
logic seems to have an assumption that the "existing" table entries are
deleted always AFTER the "desired" entries. This assumption is not true
after the introduction of ovn-ofctrl-wait-before-clear feature.
The fix here is to introduce a reference between the desired and
existing entries, so that when deleting an entry in either of the tables
it knows if the ID is still in use by its peer and decide if it is the
right time to clear the bit from the bitmap, without depending on the
order of deletion.
Fixes: 896adfd2d8b3 ("ofctrl: Support ovn-ofctrl-wait-before-clear to reduce
down time during upgrade.")
Signed-off-by: Han Zhou
---
lib/extend-table.c | 67 +
lib/extend-table.h | 17 ---
tests/ovn-controller.at | 26 +++-
3 files changed, 73 insertions(+), 37 deletions(-)
diff --git a/lib/extend-table.c b/lib/extend-table.c
index 4c3c4fac2..453b4468a 100644
--- a/lib/extend-table.c
+++ b/lib/extend-table.c
@@ -40,13 +40,17 @@ ovn_extend_table_init(struct ovn_extend_table *table)
}
static struct ovn_extend_table_info *
-ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id,
+ovn_extend_table_info_alloc(const char *name, uint32_t id,
+struct ovn_extend_table_info *peer,
uint32_t hash)
{
struct ovn_extend_table_info *e = xmalloc(sizeof *e);
e->name = xstrdup(name);
e->table_id = id;
-e->new_table_id = is_new_id;
+e->peer = peer;
+if (peer) {
+peer->peer = e;
+}
e->hmap_node.hash = hash;
hmap_init(&e->references);
return e;
@@ -184,9 +188,10 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
bool existing)
/* Clear the target table. */
HMAP_FOR_EACH_SAFE (g, hmap_node, target) {
hmap_remove(target, &g->hmap_node);
-/* Don't unset bitmap for desired group_info if the group_id
- * was not freshly reserved. */
-if (existing || g->new_table_id) {
+if (g->peer) {
+g->peer->peer = NULL;
+} else {
+/* Don't unset bitmap if the peer table is still using it. */
bitmap_set0(table->table_ids, g->table_id);
}
ovn_extend_table_info_destroy(g);
@@ -209,11 +214,15 @@ void
ovn_extend_table_remove_existing(struct ovn_extend_table *table,
struct ovn_extend_table_info *existing)
{
-/* Remove 'existing' from 'groups->existing' */
+/* Remove 'existing' from 'table->existing' */
hmap_remove(&table->existing, &existing->hmap_node);
-/* Dealloc group_id. */
-bitmap_set0(table->table_ids, existing->table_id);
+if (existing->peer) {
+existing->peer->peer = NULL;
+} else {
+/* Dealloc the ID. */
+bitmap_set0(table->table_ids, existing->table_id);
+}
ovn_extend_table_info_destroy(existing);
}
@@ -230,7 +239,9 @@ ovn_extend_table_delete_desired(struct ovn_extend_table
*table,
VLOG_DBG("%s: %s, "UUID_FMT, __func__,
e->name, UUID_ARGS(&l->lflow_uuid));
hmap_remove(&table->desired, &e->hmap_node);
-if (e-
