[ovs-dev,2/2] ovn-northd; Treat logical ports of router type as always being up

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

Commit Message

Jakub Sitnicki Sept. 29, 2017, 3:05 p.m.
Employ the simplest possible approach to determine the state of logical
ports that connect to logical routers by hardcoding it to always up.
This is intended to be less surprising than the current approach where
router ports appear as being down (with the exception of ones linking to
gateway routers, which are bound).

Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-August/045202.html
Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
---
 ovn/northd/ovn-northd.c |  2 +-
 ovn/ovn-nb.xml          | 26 +++++++++++++------
 tests/ovn-northd.at     | 69 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 9 deletions(-)

Comments

Mark Michelson Sept. 29, 2017, 7:01 p.m. | #1
LGTM

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

> Employ the simplest possible approach to determine the state of logical
> ports that connect to logical routers by hardcoding it to always up.
> This is intended to be less surprising than the current approach where
> router ports appear as being down (with the exception of ones linking to
> gateway routers, which are bound).
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2017-August/045202.html
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
>
Acked-by: Mark Michelson <mmichels@redhat.com>

> ---
>  ovn/northd/ovn-northd.c |  2 +-
>  ovn/ovn-nb.xml          | 26 +++++++++++++------
>  tests/ovn-northd.at     | 69
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 88 insertions(+), 9 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 37651a0..791da1e 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -5980,7 +5980,7 @@ update_logical_port_status(struct northd_context
> *ctx)
>              continue;
>          }
>
> -        bool up = sb->chassis ? true : false;
> +        bool up = (sb->chassis || !strcmp(nbsp->type, "router"));
>          if (!nbsp->up || *nbsp->up != up) {
>              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
>          }
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 9869d7e..5b02269 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -513,14 +513,24 @@
>
>      <group title="Port State">
>        <column name="up">
> -        This column is populated by <code>ovn-northd</code>, rather than
> by the
> -        CMS plugin as is most of this database.  When a logical port is
> bound
> -        to a physical location in the OVN Southbound database <ref
> -        db="OVN_Southbound" table="Binding"/> table,
> <code>ovn-northd</code>
> -        sets this column to <code>true</code>; otherwise, or if the port
> -        becomes unbound later, it sets it to <code>false</code>.  This
> allows
> -        the CMS to wait for a VM's (or container's) networking to become
> active
> -        before it allows the VM (or container) to start.
> +        <p>
> +          This column is populated by <code>ovn-northd</code>, rather
> +          than by the CMS plugin as is most of this database.  When a
> +          logical port is bound to a physical location in the OVN
> +          Southbound database <ref db="OVN_Southbound"
> +          table="Binding"/> table, <code>ovn-northd</code> sets this
> +          column to <code>true</code>; otherwise, or if the port
> +          becomes unbound later, it sets it to <code>false</code>.
> +          This allows the CMS to wait for a VM's (or container's)
> +          networking to become active before it allows the VM (or
> +          container) to start.
> +        </p>
> +
> +        <p>
> +          Logical ports of router type are an exception to this rule.
> +          They are considered to be always up, that is this column is
> +          always set to <code>true</code>.
> +        </p>
>        </column>
>
>        <column name="enabled">
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index fc9eda8..954e259 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -83,3 +83,72 @@ ovn-nbctl --wait=sb remove Logical_Router_Port bob
> options redirect-chassis
>  AT_CHECK([ovn-sbctl find Gateway_Chassis name=bob_gw1], [0], [])
>
>  AT_CLEANUP
> +
> +AT_SETUP([ovn -- check up state of VIF LSP])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +ovn-nbctl ls-add S1
> +ovn-nbctl lsp-add S1 S1-vm1
> +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xdown])
> +
> +ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> +ovn-sbctl lsp-bind S1-vm1 hv1
> +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xup])
> +
> +AT_CLEANUP
> +
> +AT_SETUP([ovn -- check up state of router LSP linked to a distributed LR])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +ovn-nbctl lr-add R1
> +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> +
> +ovn-nbctl ls-add S1
> +ovn-nbctl lsp-add S1 S1-R1
> +ovn-nbctl lsp-set-type S1-R1 router
> +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> +
> +AT_CLEANUP
> +
> +AT_SETUP([ovn -- check up state of router LSP linked to a gateway LR])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> +
> +ovn-nbctl create Logical_Router name=R1 options:chassis=gw1
> +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> +
> +ovn-nbctl ls-add S1
> +ovn-nbctl lsp-add S1 S1-R1
> +ovn-nbctl lsp-set-type S1-R1 router
> +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> +
> +ovn-sbctl lsp-bind S1-R1 gw1
> +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> +
> +AT_CLEANUP
> +
> +AT_SETUP([ovn -- check up state of router LSP linked to an LRP with set
> Gateway Chassis])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +ovn_start
> +
> +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> +
> +ovn-nbctl lr-add R1
> +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> +ovn-nbctl lrp-set-gateway-chassis R1-S1 gw1
> +
> +ovn-nbctl ls-add S1
> +ovn-nbctl lsp-add S1 S1-R1
> +ovn-nbctl lsp-set-type S1-R1 router
> +ovn-nbctl lsp-set-addresses S1-R1 router
> +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> +
> +AT_CLEANUP
> --
> 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:39 a.m. | #2
On Fri, Sep 29, 2017 at 9:01 PM, Mark Michelson <mmichels@redhat.com> wrote:

> LGTM
>
> On Fri, Sep 29, 2017 at 10:07 AM Jakub Sitnicki <jkbs@redhat.com> wrote:
>
> > Employ the simplest possible approach to determine the state of logical
> > ports that connect to logical routers by hardcoding it to always up.
> > This is intended to be less surprising than the current approach where
> > router ports appear as being down (with the exception of ones linking to
> > gateway routers, which are bound).
> >
> > Reported-at:
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2017-
> August/045202.html
> > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
> >
> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> > ---
> >  ovn/northd/ovn-northd.c |  2 +-
> >  ovn/ovn-nb.xml          | 26 +++++++++++++------
> >  tests/ovn-northd.at     | 69
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 88 insertions(+), 9 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 37651a0..791da1e 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -5980,7 +5980,7 @@ update_logical_port_status(struct northd_context
> > *ctx)
> >              continue;
> >          }
> >
> > -        bool up = sb->chassis ? true : false;
> > +        bool up = (sb->chassis || !strcmp(nbsp->type, "router"));
>

doesn't this introduce a functional change for ports related to gateway
routers (which are boundable?) Could we exclude those from the "always up"
and let it happen normally?

We were counting on the up/down state of those ports to realize the master
of active/backup gateway chassis sets.



>          if (!nbsp->up || *nbsp->up != up) {
> >              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> >          }
> > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> > index 9869d7e..5b02269 100644
> > --- a/ovn/ovn-nb.xml
> > +++ b/ovn/ovn-nb.xml
> > @@ -513,14 +513,24 @@
> >
> >      <group title="Port State">
> >        <column name="up">
> > -        This column is populated by <code>ovn-northd</code>, rather than
> > by the
> > -        CMS plugin as is most of this database.  When a logical port is
> > bound
> > -        to a physical location in the OVN Southbound database <ref
> > -        db="OVN_Southbound" table="Binding"/> table,
> > <code>ovn-northd</code>
> > -        sets this column to <code>true</code>; otherwise, or if the port
> > -        becomes unbound later, it sets it to <code>false</code>.  This
> > allows
> > -        the CMS to wait for a VM's (or container's) networking to become
> > active
> > -        before it allows the VM (or container) to start.
> > +        <p>
> > +          This column is populated by <code>ovn-northd</code>, rather
> > +          than by the CMS plugin as is most of this database.  When a
> > +          logical port is bound to a physical location in the OVN
> > +          Southbound database <ref db="OVN_Southbound"
> > +          table="Binding"/> table, <code>ovn-northd</code> sets this
> > +          column to <code>true</code>; otherwise, or if the port
> > +          becomes unbound later, it sets it to <code>false</code>.
> > +          This allows the CMS to wait for a VM's (or container's)
> > +          networking to become active before it allows the VM (or
> > +          container) to start.
> > +        </p>
> > +
> > +        <p>
> > +          Logical ports of router type are an exception to this rule.
> > +          They are considered to be always up, that is this column is
> > +          always set to <code>true</code>.
> > +        </p>
> >        </column>
> >
> >        <column name="enabled">
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index fc9eda8..954e259 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -83,3 +83,72 @@ ovn-nbctl --wait=sb remove Logical_Router_Port bob
> > options redirect-chassis
> >  AT_CHECK([ovn-sbctl find Gateway_Chassis name=bob_gw1], [0], [])
> >
> >  AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- check up state of VIF LSP])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +
> > +ovn-nbctl ls-add S1
> > +ovn-nbctl lsp-add S1 S1-vm1
> > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xdown])
> > +
> > +ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> > +ovn-sbctl lsp-bind S1-vm1 hv1
> > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xup])
> > +
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- check up state of router LSP linked to a distributed
> LR])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +
> > +ovn-nbctl lr-add R1
> > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > +
> > +ovn-nbctl ls-add S1
> > +ovn-nbctl lsp-add S1 S1-R1
> > +ovn-nbctl lsp-set-type S1-R1 router
> > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > +
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- check up state of router LSP linked to a gateway LR])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +
> > +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> > +
> > +ovn-nbctl create Logical_Router name=R1 options:chassis=gw1
> > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > +
> > +ovn-nbctl ls-add S1
> > +ovn-nbctl lsp-add S1 S1-R1
> > +ovn-nbctl lsp-set-type S1-R1 router
> > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > +
> > +ovn-sbctl lsp-bind S1-R1 gw1
> > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > +
> > +AT_CLEANUP
> > +
> > +AT_SETUP([ovn -- check up state of router LSP linked to an LRP with set
> > Gateway Chassis])
> > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > +ovn_start
> > +
> > +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> > +
> > +ovn-nbctl lr-add R1
> > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > +ovn-nbctl lrp-set-gateway-chassis R1-S1 gw1
> > +
> > +ovn-nbctl ls-add S1
> > +ovn-nbctl lsp-add S1 S1-R1
> > +ovn-nbctl lsp-set-type S1-R1 router
> > +ovn-nbctl lsp-set-addresses S1-R1 router
> > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > +
> > +AT_CLEANUP
> > --
> > 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
>
Jakub Sitnicki Oct. 2, 2017, 1:39 p.m. | #3
Hi Ajo,

On Mon, 2 Oct 2017 13:39:03 +0200
Miguel Angel Ajo Pelayo <majopela@redhat.com> wrote:

> On Fri, Sep 29, 2017 at 9:01 PM, Mark Michelson <mmichels@redhat.com> wrote:
> 
> > LGTM
> >
> > On Fri, Sep 29, 2017 at 10:07 AM Jakub Sitnicki <jkbs@redhat.com> wrote:
> >  
> > > Employ the simplest possible approach to determine the state of logical
> > > ports that connect to logical routers by hardcoding it to always up.
> > > This is intended to be less surprising than the current approach where
> > > router ports appear as being down (with the exception of ones linking to
> > > gateway routers, which are bound).
> > >
> > > Reported-at:
> > > https://mail.openvswitch.org/pipermail/ovs-discuss/2017-  
> > August/045202.html  
> > > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
> > >  
> > Acked-by: Mark Michelson <mmichels@redhat.com>
> >  
> > > ---
> > >  ovn/northd/ovn-northd.c |  2 +-
> > >  ovn/ovn-nb.xml          | 26 +++++++++++++------
> > >  tests/ovn-northd.at     | 69
> > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 88 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > > index 37651a0..791da1e 100644
> > > --- a/ovn/northd/ovn-northd.c
> > > +++ b/ovn/northd/ovn-northd.c
> > > @@ -5980,7 +5980,7 @@ update_logical_port_status(struct northd_context
> > > *ctx)
> > >              continue;
> > >          }
> > >
> > > -        bool up = sb->chassis ? true : false;
> > > +        bool up = (sb->chassis || !strcmp(nbsp->type, "router"));  
> >  
> 
> doesn't this introduce a functional change for ports related to gateway
> routers (which are boundable?) Could we exclude those from the "always up"
> and let it happen normally?
> 
> We were counting on the up/down state of those ports to realize the master
> of active/backup gateway chassis sets.

Yes, there is a functional change for logical ports that link to a
gateway router (that is one in Logical_Router table that has
options:chassis set). The newly added "ovn -- check up state of router
LSP linked to a gateway LR" test aims to exercise this scenario.

However, as I understand, gateway routers are a legacy way of providing
connectivity to an external network. They were superseded by LRPs
with as associated gateway chassis. (Corresponding test - "check up
state of router LSP linked to an LRP with set Gateway Chassis".)

For these, LSPs linked to one or more Gateway_Chassis, there is no
functionality loss. They currently appear as always down, and with this
patchset will appear as always up.

We could introduce an additional look-up in
update_logical_port_status() to locate the LRP that corresponds to a
LSP to take into account the existence Gateway_Chassis binding.

I would probably do this on top of this patch in a separate series - as
a further improvement but just for logical ports associated with gateway
chassis this time.

Would that be useful enough to justify an additional look-up each time
when we process a change in SB DB from ovn-northd's main loop?

Thanks,
Jakub

> 
> 
> 
> >          if (!nbsp->up || *nbsp->up != up) {  
> > >              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > >          }
> > > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> > > index 9869d7e..5b02269 100644
> > > --- a/ovn/ovn-nb.xml
> > > +++ b/ovn/ovn-nb.xml
> > > @@ -513,14 +513,24 @@
> > >
> > >      <group title="Port State">
> > >        <column name="up">
> > > -        This column is populated by <code>ovn-northd</code>, rather than
> > > by the
> > > -        CMS plugin as is most of this database.  When a logical port is
> > > bound
> > > -        to a physical location in the OVN Southbound database <ref
> > > -        db="OVN_Southbound" table="Binding"/> table,
> > > <code>ovn-northd</code>
> > > -        sets this column to <code>true</code>; otherwise, or if the port
> > > -        becomes unbound later, it sets it to <code>false</code>.  This
> > > allows
> > > -        the CMS to wait for a VM's (or container's) networking to become
> > > active
> > > -        before it allows the VM (or container) to start.
> > > +        <p>
> > > +          This column is populated by <code>ovn-northd</code>, rather
> > > +          than by the CMS plugin as is most of this database.  When a
> > > +          logical port is bound to a physical location in the OVN
> > > +          Southbound database <ref db="OVN_Southbound"
> > > +          table="Binding"/> table, <code>ovn-northd</code> sets this
> > > +          column to <code>true</code>; otherwise, or if the port
> > > +          becomes unbound later, it sets it to <code>false</code>.
> > > +          This allows the CMS to wait for a VM's (or container's)
> > > +          networking to become active before it allows the VM (or
> > > +          container) to start.
> > > +        </p>
> > > +
> > > +        <p>
> > > +          Logical ports of router type are an exception to this rule.
> > > +          They are considered to be always up, that is this column is
> > > +          always set to <code>true</code>.
> > > +        </p>
> > >        </column>
> > >
> > >        <column name="enabled">
> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > index fc9eda8..954e259 100644
> > > --- a/tests/ovn-northd.at
> > > +++ b/tests/ovn-northd.at
> > > @@ -83,3 +83,72 @@ ovn-nbctl --wait=sb remove Logical_Router_Port bob
> > > options redirect-chassis
> > >  AT_CHECK([ovn-sbctl find Gateway_Chassis name=bob_gw1], [0], [])
> > >
> > >  AT_CLEANUP
> > > +
> > > +AT_SETUP([ovn -- check up state of VIF LSP])
> > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > +ovn_start
> > > +
> > > +ovn-nbctl ls-add S1
> > > +ovn-nbctl lsp-add S1 S1-vm1
> > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xdown])
> > > +
> > > +ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> > > +ovn-sbctl lsp-bind S1-vm1 hv1
> > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xup])
> > > +
> > > +AT_CLEANUP
> > > +
> > > +AT_SETUP([ovn -- check up state of router LSP linked to a distributed  
> > LR])  
> > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > +ovn_start
> > > +
> > > +ovn-nbctl lr-add R1
> > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > > +
> > > +ovn-nbctl ls-add S1
> > > +ovn-nbctl lsp-add S1 S1-R1
> > > +ovn-nbctl lsp-set-type S1-R1 router
> > > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > > +
> > > +AT_CLEANUP
> > > +
> > > +AT_SETUP([ovn -- check up state of router LSP linked to a gateway LR])
> > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > +ovn_start
> > > +
> > > +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> > > +
> > > +ovn-nbctl create Logical_Router name=R1 options:chassis=gw1
> > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > > +
> > > +ovn-nbctl ls-add S1
> > > +ovn-nbctl lsp-add S1 S1-R1
> > > +ovn-nbctl lsp-set-type S1-R1 router
> > > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > > +
> > > +ovn-sbctl lsp-bind S1-R1 gw1
> > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > > +
> > > +AT_CLEANUP
> > > +
> > > +AT_SETUP([ovn -- check up state of router LSP linked to an LRP with set
> > > Gateway Chassis])
> > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > +ovn_start
> > > +
> > > +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> > > +
> > > +ovn-nbctl lr-add R1
> > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > > +ovn-nbctl lrp-set-gateway-chassis R1-S1 gw1
> > > +
> > > +ovn-nbctl ls-add S1
> > > +ovn-nbctl lsp-add S1 S1-R1
> > > +ovn-nbctl lsp-set-type S1-R1 router
> > > +ovn-nbctl lsp-set-addresses S1-R1 router
> > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > > +
> > > +AT_CLEANUP
> > > --
> > > 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. 2, 2017, 4:25 p.m. | #4
Acked-by: Miguel Angel Ajo <majopela@redhat.com>

On Mon, Oct 2, 2017 at 3:39 PM, Jakub Sitnicki <jkbs@redhat.com> wrote:

> Hi Ajo,
>
> On Mon, 2 Oct 2017 13:39:03 +0200
> Miguel Angel Ajo Pelayo <majopela@redhat.com> wrote:
>
> > On Fri, Sep 29, 2017 at 9:01 PM, Mark Michelson <mmichels@redhat.com>
> wrote:
> >
> > > LGTM
> > >
> > > On Fri, Sep 29, 2017 at 10:07 AM Jakub Sitnicki <jkbs@redhat.com>
> wrote:
> > >
> > > > Employ the simplest possible approach to determine the state of
> logical
> > > > ports that connect to logical routers by hardcoding it to always up.
> > > > This is intended to be less surprising than the current approach
> where
> > > > router ports appear as being down (with the exception of ones
> linking to
> > > > gateway routers, which are bound).
> > > >
> > > > Reported-at:
> > > > https://mail.openvswitch.org/pipermail/ovs-discuss/2017-
> > > August/045202.html
> > > > Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
> > > >
> > > Acked-by: Mark Michelson <mmichels@redhat.com>
> > >
> > > > ---
> > > >  ovn/northd/ovn-northd.c |  2 +-
> > > >  ovn/ovn-nb.xml          | 26 +++++++++++++------
> > > >  tests/ovn-northd.at     | 69
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 88 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > > > index 37651a0..791da1e 100644
> > > > --- a/ovn/northd/ovn-northd.c
> > > > +++ b/ovn/northd/ovn-northd.c
> > > > @@ -5980,7 +5980,7 @@ update_logical_port_status(struct
> northd_context
> > > > *ctx)
> > > >              continue;
> > > >          }
> > > >
> > > > -        bool up = sb->chassis ? true : false;
> > > > +        bool up = (sb->chassis || !strcmp(nbsp->type, "router"));
> > >
> >
> > doesn't this introduce a functional change for ports related to gateway
> > routers (which are boundable?) Could we exclude those from the "always
> up"
> > and let it happen normally?
> >
> > We were counting on the up/down state of those ports to realize the
> master
> > of active/backup gateway chassis sets.
>
> Yes, there is a functional change for logical ports that link to a
> gateway router (that is one in Logical_Router table that has
> options:chassis set). The newly added "ovn -- check up state of router
> LSP linked to a gateway LR" test aims to exercise this scenario.
>
> However, as I understand, gateway routers are a legacy way of providing
> connectivity to an external network. They were superseded by LRPs
> with as associated gateway chassis. (Corresponding test - "check up
> state of router LSP linked to an LRP with set Gateway Chassis".)
>
>
Thank you for the explanation, it makes sense.


> For these, LSPs linked to one or more Gateway_Chassis, there is no
> functionality loss. They currently appear as always down, and with this
> patchset will appear as always up.


> We could introduce an additional look-up in
> update_logical_port_status() to locate the LRP that corresponds to a
> LSP to take into account the existence Gateway_Chassis binding.
>
> I would probably do this on top of this patch in a separate series - as
> a further improvement but just for logical ports associated with gateway
> chassis this time.
>
> Would that be useful enough to justify an additional look-up each time
> when we process a change in SB DB from ovn-northd's main loop?
>
>
It's probably not worth it. I was thinking that we could look up on the
SBDB for the chassisredirect port that we derive out of the lrp/lsp in this
case. But what we finally get is a simple up/down state which is not very
helpful.

What would be helpful in neutron would be getting to know which chassis is
active. But we can do that via the SBDB connection we also keep.


I'm acking the patch based on the discussion.


> Thanks,
> Jakub
>
> >
> >
> >
> > >          if (!nbsp->up || *nbsp->up != up) {
> > > >              nbrec_logical_switch_port_set_up(nbsp, &up, 1);
> > > >          }
> > > > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> > > > index 9869d7e..5b02269 100644
> > > > --- a/ovn/ovn-nb.xml
> > > > +++ b/ovn/ovn-nb.xml
> > > > @@ -513,14 +513,24 @@
> > > >
> > > >      <group title="Port State">
> > > >        <column name="up">
> > > > -        This column is populated by <code>ovn-northd</code>, rather
> than
> > > > by the
> > > > -        CMS plugin as is most of this database.  When a logical
> port is
> > > > bound
> > > > -        to a physical location in the OVN Southbound database <ref
> > > > -        db="OVN_Southbound" table="Binding"/> table,
> > > > <code>ovn-northd</code>
> > > > -        sets this column to <code>true</code>; otherwise, or if the
> port
> > > > -        becomes unbound later, it sets it to <code>false</code>.
> This
> > > > allows
> > > > -        the CMS to wait for a VM's (or container's) networking to
> become
> > > > active
> > > > -        before it allows the VM (or container) to start.
> > > > +        <p>
> > > > +          This column is populated by <code>ovn-northd</code>,
> rather
> > > > +          than by the CMS plugin as is most of this database.  When
> a
> > > > +          logical port is bound to a physical location in the OVN
> > > > +          Southbound database <ref db="OVN_Southbound"
> > > > +          table="Binding"/> table, <code>ovn-northd</code> sets this
> > > > +          column to <code>true</code>; otherwise, or if the port
> > > > +          becomes unbound later, it sets it to <code>false</code>.
> > > > +          This allows the CMS to wait for a VM's (or container's)
> > > > +          networking to become active before it allows the VM (or
> > > > +          container) to start.
> > > > +        </p>
> > > > +
> > > > +        <p>
> > > > +          Logical ports of router type are an exception to this
> rule.
> > > > +          They are considered to be always up, that is this column
> is
> > > > +          always set to <code>true</code>.
> > > > +        </p>
> > > >        </column>
> > > >
> > > >        <column name="enabled">
> > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > index fc9eda8..954e259 100644
> > > > --- a/tests/ovn-northd.at
> > > > +++ b/tests/ovn-northd.at
> > > > @@ -83,3 +83,72 @@ ovn-nbctl --wait=sb remove Logical_Router_Port bob
> > > > options redirect-chassis
> > > >  AT_CHECK([ovn-sbctl find Gateway_Chassis name=bob_gw1], [0], [])
> > > >
> > > >  AT_CLEANUP
> > > > +
> > > > +AT_SETUP([ovn -- check up state of VIF LSP])
> > > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > > +ovn_start
> > > > +
> > > > +ovn-nbctl ls-add S1
> > > > +ovn-nbctl lsp-add S1 S1-vm1
> > > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xdown])
> > > > +
> > > > +ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> > > > +ovn-sbctl lsp-bind S1-vm1 hv1
> > > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xup])
> > > > +
> > > > +AT_CLEANUP
> > > > +
> > > > +AT_SETUP([ovn -- check up state of router LSP linked to a
> distributed
> > > LR])
> > > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > > +ovn_start
> > > > +
> > > > +ovn-nbctl lr-add R1
> > > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > > > +
> > > > +ovn-nbctl ls-add S1
> > > > +ovn-nbctl lsp-add S1 S1-R1
> > > > +ovn-nbctl lsp-set-type S1-R1 router
> > > > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> > > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > > > +
> > > > +AT_CLEANUP
> > > > +
> > > > +AT_SETUP([ovn -- check up state of router LSP linked to a gateway
> LR])
> > > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > > +ovn_start
> > > > +
> > > > +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> > > > +
> > > > +ovn-nbctl create Logical_Router name=R1 options:chassis=gw1
> > > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > > > +
> > > > +ovn-nbctl ls-add S1
> > > > +ovn-nbctl lsp-add S1 S1-R1
> > > > +ovn-nbctl lsp-set-type S1-R1 router
> > > > +ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
> > > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > > > +
> > > > +ovn-sbctl lsp-bind S1-R1 gw1
> > > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > > > +
> > > > +AT_CLEANUP
> > > > +
> > > > +AT_SETUP([ovn -- check up state of router LSP linked to an LRP with
> set
> > > > Gateway Chassis])
> > > > +AT_SKIP_IF([test $HAVE_PYTHON = no])
> > > > +ovn_start
> > > > +
> > > > +ovn-sbctl chassis-add gw1 geneve 127.0.0.1
> > > > +
> > > > +ovn-nbctl lr-add R1
> > > > +ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
> > > > +ovn-nbctl lrp-set-gateway-chassis R1-S1 gw1
> > > > +
> > > > +ovn-nbctl ls-add S1
> > > > +ovn-nbctl lsp-add S1 S1-R1
> > > > +ovn-nbctl lsp-set-type S1-R1 router
> > > > +ovn-nbctl lsp-set-addresses S1-R1 router
> > > > +ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
> > > > +AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> > > > +
> > > > +AT_CLEANUP
> > > > --
> > > > 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 37651a0..791da1e 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5980,7 +5980,7 @@  update_logical_port_status(struct northd_context *ctx)
             continue;
         }
 
-        bool up = sb->chassis ? true : false;
+        bool up = (sb->chassis || !strcmp(nbsp->type, "router"));
         if (!nbsp->up || *nbsp->up != up) {
             nbrec_logical_switch_port_set_up(nbsp, &up, 1);
         }
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 9869d7e..5b02269 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -513,14 +513,24 @@ 
 
     <group title="Port State">
       <column name="up">
-        This column is populated by <code>ovn-northd</code>, rather than by the
-        CMS plugin as is most of this database.  When a logical port is bound
-        to a physical location in the OVN Southbound database <ref
-        db="OVN_Southbound" table="Binding"/> table, <code>ovn-northd</code>
-        sets this column to <code>true</code>; otherwise, or if the port
-        becomes unbound later, it sets it to <code>false</code>.  This allows
-        the CMS to wait for a VM's (or container's) networking to become active
-        before it allows the VM (or container) to start.
+        <p>
+          This column is populated by <code>ovn-northd</code>, rather
+          than by the CMS plugin as is most of this database.  When a
+          logical port is bound to a physical location in the OVN
+          Southbound database <ref db="OVN_Southbound"
+          table="Binding"/> table, <code>ovn-northd</code> sets this
+          column to <code>true</code>; otherwise, or if the port
+          becomes unbound later, it sets it to <code>false</code>.
+          This allows the CMS to wait for a VM's (or container's)
+          networking to become active before it allows the VM (or
+          container) to start.
+        </p>
+
+        <p>
+          Logical ports of router type are an exception to this rule.
+          They are considered to be always up, that is this column is
+          always set to <code>true</code>.
+        </p>
       </column>
 
       <column name="enabled">
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index fc9eda8..954e259 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -83,3 +83,72 @@  ovn-nbctl --wait=sb remove Logical_Router_Port bob options redirect-chassis
 AT_CHECK([ovn-sbctl find Gateway_Chassis name=bob_gw1], [0], [])
 
 AT_CLEANUP
+
+AT_SETUP([ovn -- check up state of VIF LSP])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+ovn-nbctl ls-add S1
+ovn-nbctl lsp-add S1 S1-vm1
+AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xdown])
+
+ovn-sbctl chassis-add hv1 geneve 127.0.0.1
+ovn-sbctl lsp-bind S1-vm1 hv1
+AT_CHECK([test x`ovn-nbctl lsp-get-up S1-vm1` = xup])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- check up state of router LSP linked to a distributed LR])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+ovn-nbctl lr-add R1
+ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
+
+ovn-nbctl ls-add S1
+ovn-nbctl lsp-add S1 S1-R1
+ovn-nbctl lsp-set-type S1-R1 router
+ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
+ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
+AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- check up state of router LSP linked to a gateway LR])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+
+ovn-nbctl create Logical_Router name=R1 options:chassis=gw1
+ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
+
+ovn-nbctl ls-add S1
+ovn-nbctl lsp-add S1 S1-R1
+ovn-nbctl lsp-set-type S1-R1 router
+ovn-nbctl lsp-set-addresses S1-R1 02:ac:10:01:00:01
+ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
+
+ovn-sbctl lsp-bind S1-R1 gw1
+AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- check up state of router LSP linked to an LRP with set Gateway Chassis])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+ovn-sbctl chassis-add gw1 geneve 127.0.0.1
+
+ovn-nbctl lr-add R1
+ovn-nbctl lrp-add R1 R1-S1 02:ac:10:01:00:01 172.16.1.1/24
+ovn-nbctl lrp-set-gateway-chassis R1-S1 gw1
+
+ovn-nbctl ls-add S1
+ovn-nbctl lsp-add S1 S1-R1
+ovn-nbctl lsp-set-type S1-R1 router
+ovn-nbctl lsp-set-addresses S1-R1 router
+ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1
+AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
+
+AT_CLEANUP