Acked-by: Miguel Angel Ajo <majop...@redhat.com> On Mon, Oct 2, 2017 at 3:39 PM, Jakub Sitnicki <j...@redhat.com> wrote:
> Hi Ajo, > > On Mon, 2 Oct 2017 13:39:03 +0200 > Miguel Angel Ajo Pelayo <majop...@redhat.com> wrote: > > > On Fri, Sep 29, 2017 at 9:01 PM, Mark Michelson <mmich...@redhat.com> > wrote: > > > > > LGTM > > > > > > On Fri, Sep 29, 2017 at 10:07 AM Jakub Sitnicki <j...@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 <j...@redhat.com> > > > > > > > Acked-by: Mark Michelson <mmich...@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 > > > > d...@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > > > dev mailing list > > > d...@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
