[ovs-dev,1/2] ovn-northd: Refactor logic for logical port 'up' state update

Message ID 20170929150523.8833-2-jkbs@redhat.com
State Accepted
Headers show
Series
  • Make router ports always appear as up
Related show

Commit Message

Jakub Sitnicki Sept. 29, 2017, 3:05 p.m.
No functional change.  Make it obvious that we determine the logical
port 'up' state by checking for bound chassis, and update the NB DB only
when state has not been set yet or current state is different.

Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
---
 ovn/northd/ovn-northd.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Mark Michelson Sept. 29, 2017, 6:06 p.m. | #1
This looks much better. I wrote out the truth table to confirm that the
functionality is the same.

On Fri, Sep 29, 2017 at 10:06 AM Jakub Sitnicki <jkbs@redhat.com> wrote:

> No functional change.  Make it obvious that we determine the logical
> port 'up' state by checking for bound chassis, and update the NB DB only
> when state has not been set yet or current state is different.
>
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
>
Acked-by: Mark Michelson <mmichels@redhat.com>

> ---
>  ovn/northd/ovn-northd.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 2db2380..37651a0 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -5980,11 +5980,8 @@ update_logical_port_status(struct northd_context
> *ctx)
>              continue;
>          }
>
> -        if (sb->chassis && (!nbsp->up || !*nbsp->up)) {
> -            bool up = true;
> -            nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> -        } else if (!sb->chassis && (!nbsp->up || *nbsp->up)) {
> -            bool up = false;
> +        bool up = sb->chassis ? true : false;
> +        if (!nbsp->up || *nbsp->up != up) {
>              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
>          }
>      }
> --
> 2.9.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Miguel Angel Ajo Pelayo Oct. 2, 2017, 11:33 a.m. | #2
Acked-by: Miguel Angel Ajo <majopela@redhat.com>

On Fri, Sep 29, 2017 at 8:06 PM, Mark Michelson <mmichels@redhat.com> wrote:

> This looks much better. I wrote out the truth table to confirm that the
> functionality is the same.
>
> On Fri, Sep 29, 2017 at 10:06 AM Jakub Sitnicki <jkbs@redhat.com> wrote:
>
> > No functional change.  Make it obvious that we determine the logical
> > port 'up' state by checking for bound chassis, and update the NB DB only
> > when state has not been set yet or current state is different.
> >
> > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
> >
> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> > ---
> >  ovn/northd/ovn-northd.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 2db2380..37651a0 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -5980,11 +5980,8 @@ update_logical_port_status(struct northd_context
> > *ctx)
> >              continue;
> >          }
> >
> > -        if (sb->chassis && (!nbsp->up || !*nbsp->up)) {
> > -            bool up = true;
> > -            nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > -        } else if (!sb->chassis && (!nbsp->up || *nbsp->up)) {
> > -            bool up = false;
> > +        bool up = sb->chassis ? true : false;
> > +        if (!nbsp->up || *nbsp->up != up) {
> >              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> >          }
> >      }
> > --
> > 2.9.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Miguel Angel Ajo Pelayo Oct. 27, 2017, 1:21 p.m. | #3
Can we move this patches forward and get a backport to 2.8 / 2.7 branches?

Otherwise we get a failure on openstack's:

neutron.tests.tempest.api.test_routers.RoutersTest.test_router_interface_status

On Mon, Oct 2, 2017 at 1:33 PM, Miguel Angel Ajo Pelayo <majopela@redhat.com
> wrote:

> Acked-by: Miguel Angel Ajo <majopela@redhat.com>
>
> On Fri, Sep 29, 2017 at 8:06 PM, Mark Michelson <mmichels@redhat.com>
> wrote:
>
>> This looks much better. I wrote out the truth table to confirm that the
>> functionality is the same.
>>
>> On Fri, Sep 29, 2017 at 10:06 AM Jakub Sitnicki <jkbs@redhat.com> wrote:
>>
>> > No functional change.  Make it obvious that we determine the logical
>> > port 'up' state by checking for bound chassis, and update the NB DB only
>> > when state has not been set yet or current state is different.
>> >
>> > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
>> >
>> Acked-by: Mark Michelson <mmichels@redhat.com>
>>
>> > ---
>> >  ovn/northd/ovn-northd.c | 7 ++-----
>> >  1 file changed, 2 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> > index 2db2380..37651a0 100644
>> > --- a/ovn/northd/ovn-northd.c
>> > +++ b/ovn/northd/ovn-northd.c
>> > @@ -5980,11 +5980,8 @@ update_logical_port_status(struct northd_context
>> > *ctx)
>> >              continue;
>> >          }
>> >
>> > -        if (sb->chassis && (!nbsp->up || !*nbsp->up)) {
>> > -            bool up = true;
>> > -            nbrec_logical_switch_port_set_up(nbsp, &up, 1);
>> > -        } else if (!sb->chassis && (!nbsp->up || *nbsp->up)) {
>> > -            bool up = false;
>> > +        bool up = sb->chassis ? true : false;
>> > +        if (!nbsp->up || *nbsp->up != up) {
>> >              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
>> >          }
>> >      }
>> > --
>> > 2.9.5
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>

Patch

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 2db2380..37651a0 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5980,11 +5980,8 @@  update_logical_port_status(struct northd_context *ctx)
             continue;
         }
 
-        if (sb->chassis && (!nbsp->up || !*nbsp->up)) {
-            bool up = true;
-            nbrec_logical_switch_port_set_up(nbsp, &up, 1);
-        } else if (!sb->chassis && (!nbsp->up || *nbsp->up)) {
-            bool up = false;
+        bool up = sb->chassis ? true : false;
+        if (!nbsp->up || *nbsp->up != up) {
             nbrec_logical_switch_port_set_up(nbsp, &up, 1);
         }
     }