| Message ID | 20211111003913.492906-1-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-nbctl: Fix lrp-set-gateway-chassis. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Nov 10, 2021 at 7:39 PM Han Zhou <hzhou@ovn.org> wrote: > > Currently this command assumes that if the gateway_chassis record with > expected name exists it is set to the logical port, so once the record > is found it not set to the lrp again. However, this assumption is not > always true. > > An example is that when combined with a lrp-del and then lrp-add > commands before lrp-set-gateway-chassis in the same transaction, the > gateway_chassis record will be found but it is not set to the lrp. This > is causing the gateway-chassis setting flapping in ovn-kubernetes' > cluster router logical ports. > > This patch makes sure the gateway_chassis record (existed or newly > created) is set to the lrp's gateway-chassis column. > > Signed-off-by: Han Zhou <hzhou@ovn.org> Acked-by: Numan Siddique <numans@ovn.org> Numan > --- > ovs | 2 +- > tests/ovn-nbctl.at | 6 ++++++ > utilities/ovn-nbctl.c | 19 ++++++++----------- > 3 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/ovs b/ovs > index 1bdda7b6d..429b114c5 160000 > --- a/ovs > +++ b/ovs > @@ -1 +1 @@ > -Subproject commit 1bdda7b6d53c92e877b457157676aff326414c53 > +Subproject commit 429b114c5aadee24ccfb16ad7d824f45cdcea75a > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index a8946fef8..3846ba727 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -1389,6 +1389,12 @@ AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 10]) > AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > lrp0-chassis1 10 > ]) > + > +AT_CHECK([ovn-nbctl lrp-del lrp0 -- lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24 -- lrp-set-gateway-chassis lrp0 chassis1 10]) > +AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > +lrp0-chassis1 10 > +]) > + > AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 20]) > > AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 2a68ccd16..72049ffd8 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -5100,24 +5100,21 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx) > } > > gc_name = xasprintf("%s-%s", lrp_name, chassis_name); > - const struct nbrec_gateway_chassis *existing_gc; > - error = gc_by_name_or_uuid(ctx, gc_name, false, &existing_gc); > + const struct nbrec_gateway_chassis *gc; > + error = gc_by_name_or_uuid(ctx, gc_name, false, &gc); > if (error) { > ctx->error = error; > free(gc_name); > return; > } > - if (existing_gc) { > - nbrec_gateway_chassis_set_priority(existing_gc, priority); > - free(gc_name); > - return; > + > + if (!gc) { > + /* Create the logical gateway chassis. */ > + gc = nbrec_gateway_chassis_insert(ctx->txn); > + nbrec_gateway_chassis_set_name(gc, gc_name); > + nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); > } > > - /* Create the logical gateway chassis. */ > - struct nbrec_gateway_chassis *gc > - = nbrec_gateway_chassis_insert(ctx->txn); > - nbrec_gateway_chassis_set_name(gc, gc_name); > - nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); > nbrec_gateway_chassis_set_priority(gc, priority); > > /* Insert the logical gateway chassis into the logical router port. */ > -- > 2.30.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hello Han, The patch looks good to me, but it updates the OVS submodule commit, and I don't think this was done on purpose. With that corrected, Acked-by: Mark Michelson <mmichels@redhat.com> On 11/10/21 19:39, Han Zhou wrote: > Currently this command assumes that if the gateway_chassis record with > expected name exists it is set to the logical port, so once the record > is found it not set to the lrp again. However, this assumption is not > always true. > > An example is that when combined with a lrp-del and then lrp-add > commands before lrp-set-gateway-chassis in the same transaction, the > gateway_chassis record will be found but it is not set to the lrp. This > is causing the gateway-chassis setting flapping in ovn-kubernetes' > cluster router logical ports. > > This patch makes sure the gateway_chassis record (existed or newly > created) is set to the lrp's gateway-chassis column. > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > ovs | 2 +- > tests/ovn-nbctl.at | 6 ++++++ > utilities/ovn-nbctl.c | 19 ++++++++----------- > 3 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/ovs b/ovs > index 1bdda7b6d..429b114c5 160000 > --- a/ovs > +++ b/ovs > @@ -1 +1 @@ > -Subproject commit 1bdda7b6d53c92e877b457157676aff326414c53 > +Subproject commit 429b114c5aadee24ccfb16ad7d824f45cdcea75a > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index a8946fef8..3846ba727 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -1389,6 +1389,12 @@ AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 10]) > AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > lrp0-chassis1 10 > ]) > + > +AT_CHECK([ovn-nbctl lrp-del lrp0 -- lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24 -- lrp-set-gateway-chassis lrp0 chassis1 10]) > +AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > +lrp0-chassis1 10 > +]) > + > AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 20]) > > AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 2a68ccd16..72049ffd8 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -5100,24 +5100,21 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx) > } > > gc_name = xasprintf("%s-%s", lrp_name, chassis_name); > - const struct nbrec_gateway_chassis *existing_gc; > - error = gc_by_name_or_uuid(ctx, gc_name, false, &existing_gc); > + const struct nbrec_gateway_chassis *gc; > + error = gc_by_name_or_uuid(ctx, gc_name, false, &gc); > if (error) { > ctx->error = error; > free(gc_name); > return; > } > - if (existing_gc) { > - nbrec_gateway_chassis_set_priority(existing_gc, priority); > - free(gc_name); > - return; > + > + if (!gc) { > + /* Create the logical gateway chassis. */ > + gc = nbrec_gateway_chassis_insert(ctx->txn); > + nbrec_gateway_chassis_set_name(gc, gc_name); > + nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); > } > > - /* Create the logical gateway chassis. */ > - struct nbrec_gateway_chassis *gc > - = nbrec_gateway_chassis_insert(ctx->txn); > - nbrec_gateway_chassis_set_name(gc, gc_name); > - nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); > nbrec_gateway_chassis_set_priority(gc, priority); > > /* Insert the logical gateway chassis into the logical router port. */ >
On Thu, Nov 11, 2021 at 11:53 AM Mark Michelson <mmichels@redhat.com> wrote: > > Hello Han, > > The patch looks good to me, but it updates the OVS submodule commit, and > I don't think this was done on purpose. With that corrected, > > Acked-by: Mark Michelson <mmichels@redhat.com> Sorry, my mistake. With the ovs submodule commit removed, I applied it to main and backported up to 21.03 branch. Thanks Numan and Mark for the reviews! Han > > On 11/10/21 19:39, Han Zhou wrote: > > Currently this command assumes that if the gateway_chassis record with > > expected name exists it is set to the logical port, so once the record > > is found it not set to the lrp again. However, this assumption is not > > always true. > > > > An example is that when combined with a lrp-del and then lrp-add > > commands before lrp-set-gateway-chassis in the same transaction, the > > gateway_chassis record will be found but it is not set to the lrp. This > > is causing the gateway-chassis setting flapping in ovn-kubernetes' > > cluster router logical ports. > > > > This patch makes sure the gateway_chassis record (existed or newly > > created) is set to the lrp's gateway-chassis column. > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > ovs | 2 +- > > tests/ovn-nbctl.at | 6 ++++++ > > utilities/ovn-nbctl.c | 19 ++++++++----------- > > 3 files changed, 15 insertions(+), 12 deletions(-) > > > > diff --git a/ovs b/ovs > > index 1bdda7b6d..429b114c5 160000 > > --- a/ovs > > +++ b/ovs > > @@ -1 +1 @@ > > -Subproject commit 1bdda7b6d53c92e877b457157676aff326414c53 > > +Subproject commit 429b114c5aadee24ccfb16ad7d824f45cdcea75a > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index a8946fef8..3846ba727 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -1389,6 +1389,12 @@ AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 10]) > > AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > > lrp0-chassis1 10 > > ]) > > + > > +AT_CHECK([ovn-nbctl lrp-del lrp0 -- lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24 -- lrp-set-gateway-chassis lrp0 chassis1 10]) > > +AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > > +lrp0-chassis1 10 > > +]) > > + > > AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 20]) > > > > AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 2a68ccd16..72049ffd8 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -5100,24 +5100,21 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx) > > } > > > > gc_name = xasprintf("%s-%s", lrp_name, chassis_name); > > - const struct nbrec_gateway_chassis *existing_gc; > > - error = gc_by_name_or_uuid(ctx, gc_name, false, &existing_gc); > > + const struct nbrec_gateway_chassis *gc; > > + error = gc_by_name_or_uuid(ctx, gc_name, false, &gc); > > if (error) { > > ctx->error = error; > > free(gc_name); > > return; > > } > > - if (existing_gc) { > > - nbrec_gateway_chassis_set_priority(existing_gc, priority); > > - free(gc_name); > > - return; > > + > > + if (!gc) { > > + /* Create the logical gateway chassis. */ > > + gc = nbrec_gateway_chassis_insert(ctx->txn); > > + nbrec_gateway_chassis_set_name(gc, gc_name); > > + nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); > > } > > > > - /* Create the logical gateway chassis. */ > > - struct nbrec_gateway_chassis *gc > > - = nbrec_gateway_chassis_insert(ctx->txn); > > - nbrec_gateway_chassis_set_name(gc, gc_name); > > - nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); > > nbrec_gateway_chassis_set_priority(gc, priority); > > > > /* Insert the logical gateway chassis into the logical router port. */ > > >
diff --git a/ovs b/ovs index 1bdda7b6d..429b114c5 160000 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 1bdda7b6d53c92e877b457157676aff326414c53 +Subproject commit 429b114c5aadee24ccfb16ad7d824f45cdcea75a diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index a8946fef8..3846ba727 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -1389,6 +1389,12 @@ AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 10]) AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl lrp0-chassis1 10 ]) + +AT_CHECK([ovn-nbctl lrp-del lrp0 -- lrp-add lr0 lrp0 00:00:00:01:02:03 192.168.1.1/24 -- lrp-set-gateway-chassis lrp0 chassis1 10]) +AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl +lrp0-chassis1 10 +]) + AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 20]) AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 2a68ccd16..72049ffd8 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -5100,24 +5100,21 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context *ctx) } gc_name = xasprintf("%s-%s", lrp_name, chassis_name); - const struct nbrec_gateway_chassis *existing_gc; - error = gc_by_name_or_uuid(ctx, gc_name, false, &existing_gc); + const struct nbrec_gateway_chassis *gc; + error = gc_by_name_or_uuid(ctx, gc_name, false, &gc); if (error) { ctx->error = error; free(gc_name); return; } - if (existing_gc) { - nbrec_gateway_chassis_set_priority(existing_gc, priority); - free(gc_name); - return; + + if (!gc) { + /* Create the logical gateway chassis. */ + gc = nbrec_gateway_chassis_insert(ctx->txn); + nbrec_gateway_chassis_set_name(gc, gc_name); + nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); } - /* Create the logical gateway chassis. */ - struct nbrec_gateway_chassis *gc - = nbrec_gateway_chassis_insert(ctx->txn); - nbrec_gateway_chassis_set_name(gc, gc_name); - nbrec_gateway_chassis_set_chassis_name(gc, chassis_name); nbrec_gateway_chassis_set_priority(gc, priority); /* Insert the logical gateway chassis into the logical router port. */
Currently this command assumes that if the gateway_chassis record with expected name exists it is set to the logical port, so once the record is found it not set to the lrp again. However, this assumption is not always true. An example is that when combined with a lrp-del and then lrp-add commands before lrp-set-gateway-chassis in the same transaction, the gateway_chassis record will be found but it is not set to the lrp. This is causing the gateway-chassis setting flapping in ovn-kubernetes' cluster router logical ports. This patch makes sure the gateway_chassis record (existed or newly created) is set to the lrp's gateway-chassis column. Signed-off-by: Han Zhou <hzhou@ovn.org> --- ovs | 2 +- tests/ovn-nbctl.at | 6 ++++++ utilities/ovn-nbctl.c | 19 ++++++++----------- 3 files changed, 15 insertions(+), 12 deletions(-)