| Message ID | 20170929150523.8833-3-jkbs@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Make router ports always appear as up | expand |
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 >
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 >
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 > >
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 > > > > >
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@redhat.com > wrote: > 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 >> > > >> >> >
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@redhat.com > > wrote: > > > 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 > >> > > > >> > >> > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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
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(-)