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

Ben Pfaff blp at ovn.org
Tue Nov 28 21:04:15 UTC 2017


Sorry about the delay.  I applied these to master and backported as far
as 2.7.

On Fri, Oct 27, 2017 at 03:22:08PM +0200, Miguel Angel Ajo Pelayo wrote:
> Can we move this patch 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
> 
> 
> Cheers & thanks.
> 
> On Mon, Oct 2, 2017 at 6:25 PM, Miguel Angel Ajo Pelayo <majopela at redhat.com
> > wrote:
> 
> > Acked-by: Miguel Angel Ajo <majopela at redhat.com>
> >
> > On Mon, Oct 2, 2017 at 3:39 PM, Jakub Sitnicki <jkbs at redhat.com> wrote:
> >
> >> Hi Ajo,
> >>
> >> On Mon, 2 Oct 2017 13:39:03 +0200
> >> Miguel Angel Ajo Pelayo <majopela at redhat.com> wrote:
> >>
> >> > On Fri, Sep 29, 2017 at 9:01 PM, Mark Michelson <mmichels at redhat.com>
> >> wrote:
> >> >
> >> > > LGTM
> >> > >
> >> > > On Fri, Sep 29, 2017 at 10:07 AM Jakub Sitnicki <jkbs at 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 at redhat.com>
> >> > > >
> >> > > Acked-by: Mark Michelson <mmichels at 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 at openvswitch.org
> >> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> > > >
> >> > > _______________________________________________
> >> > > dev mailing list
> >> > > dev at openvswitch.org
> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> > >
> >>
> >>
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list