[ovs-dev] [PATCH ovn] ovn-northd: Optionally skip the check of lsp_is_up.
Han Zhou
hzhou at ovn.org
Tue Sep 8 21:23:42 UTC 2020
On Tue, Sep 8, 2020 at 11:27 AM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Tue, Sep 8, 2020 at 5:29 AM Numan Siddique <numans at ovn.org> wrote:
> >
> > On Thu, Sep 3, 2020 at 7:44 AM Han Zhou <hzhou at ovn.org> wrote:
> > >
> > > The checking of lsp "up" status before adding ARP responder flows was
> > > added to avoid confusion in cases when a network admin sees ARP
response
> > > for VM/containers that is not up on chassis yet. However, this check
> > > introduces an extra round of flow change in SB and triggers computes
> > > on hypervisors which is unnecessary cost in most cases, especially for
> > > large scale environment. To improve this, this patch provides an
option
> > > check_lsp_is_up to disable the check (when setting to "false") for the
> > > use cases when ARP reponse for a port that is "down" isn't considered
> > > harmful.
> > >
> > > Signed-off-by: Han Zhou <hzhou at ovn.org>
> >
> >
> > Few minor comments.
> >
> > Acked-by: Numan Siddique <numans at ovn.org> with those addressed.
> >
> > Thanks
> > Numan
> >
> > > ---
> > > northd/ovn-northd.8.xml | 15 ++++++++++-----
> > > northd/ovn-northd.c | 7 ++++++-
> > > ovn-nb.xml | 18 ++++++++++++++++++
> > > tests/ovn-northd.at | 14 ++++++++++++++
> > > 4 files changed, 48 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > > index 989e364..df43eb6 100644
> > > --- a/northd/ovn-northd.8.xml
> > > +++ b/northd/ovn-northd.8.xml
> > > @@ -763,9 +763,11 @@ output;
> > >
> > > <p>
> > > These flows are omitted for logical ports (other than
router ports or
> > > - <code>localport</code> ports) that are down, for logical
ports of
> > > - type <code>virtual</code> and for logical ports with
'unknown'
> > > - address set.
> > > + <code>localport</code> ports) that are down (unless <code>
> > > + check_lsp_is_up</code> is configured as false in
<code>options</code>
> > > + column of <code>NB_Global</code> table of the
<code>Northbound</code>
> > > + database), for logical ports of type <code>virtual</code>
and for
> > > + logical ports with 'unknown' address set.
> > > </p>
> > > </li>
> > >
> > > @@ -812,8 +814,11 @@ nd_na_router {
> > >
> > > <p>
> > > These flows are omitted for logical ports (other than
router ports or
> > > - <code>localport</code> ports) that are down and for
logical ports of
> > > - type <code>virtual</code>.
> > > + <code>localport</code> ports) that are down (unless <code>
> > > + check_lsp_is_up</code> is configured as false in
<code>options</code>
> > > + column of <code>NB_Global</code> table of the
<code>Northbound</code>
> > > + database), for logical ports of type <code>virtual</code>
and for
> > > + logical ports with 'unknown' address set.
> > > </p>
> > > </li>
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 7be0e85..291713e 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -86,6 +86,8 @@ static struct eth_addr mac_prefix;
> > >
> > > static bool controller_event_en;
> > >
> > > +static bool check_lsp_is_up;
> > > +
> > > /* MAC allocated for service monitor usage. Just one mac is allocated
> > > * for this purpose and ovn-controller's on each chassis will make
use
> > > * of this mac when sending out the packets to monitor the services
> > > @@ -6739,7 +6741,8 @@ build_lswitch_flows(struct hmap *datapaths,
struct hmap *ports,
> > > * - port type is router or
> > > * - port type is localport
> > > */
> > > - if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type,
"router") &&
> > > + if (check_lsp_is_up &&
> > > + !lsp_is_up(op->nbsp) && strcmp(op->nbsp->type,
"router") &&
> > > strcmp(op->nbsp->type, "localport")) {
> > > continue;
> > > }
> > > @@ -11722,6 +11725,8 @@ ovnnb_db_run(struct northd_context *ctx,
> > >
> > > controller_event_en = smap_get_bool(&nb->options,
> > > "controller_event", false);
> > > + check_lsp_is_up = smap_get_bool(&nb->options,
> > > + "check_lsp_is_up", true);
> > >
> > > build_datapaths(ctx, datapaths, lr_list);
> > > build_ports(ctx, sbrec_chassis_by_name, datapaths, ports);
> > > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > > index 1f2dbb9..4e34518 100644
> > > --- a/ovn-nb.xml
> > > +++ b/ovn-nb.xml
> > > @@ -147,6 +147,24 @@
> > > </p>
> > > </column>
> > >
> > > + <column name="options" key="check_lsp_is_up">
> >
> > How about "ignore_lport_down" ? The default would be false. If you're
> > fine with this, then
> > you need to invert the logic in the code.
> >
> Thanks Numan for the suggestion. Yes, this sounds better. And since in
ovn-nbctl we use lsp to denote a logical switch port, maybe
"ignore_lsp_down" is better?
> I will send a v2 for this.
>
Hi Numan,
Could you please take one more look at v2:
https://patchwork.ozlabs.org/project/ovn/patch/1599600116-129820-1-git-send-email-hzhou@ovn.org/
Thanks,
Han
> > I don't have a very strong opinion. If you want to stick with
> > check_lsp_is_up, I'm fine with it.
> >
> >
> > > + <p>
> > > + If set to true, ARP/ND reply flows for logical switch
ports will be
> > > + installed only if the port is up, i.e. claimed by a
Chassis. If set
> > > + to false, these flows are installed regardless of the
status of the
> > > + port, which can result in a situation that ARP request to
an IP is
> > > + resolved even before the relevant VM/container is running.
For
> > > + environments where this is not an issue, setting it to
> > > + <code>false</code> can reduce the load and latency of the
control
> > > + plane. The default value is <code>true</code>.
> > > + </p>
> > > +
> > > + <p>
> > > + If the value is nonzero, then it will be forced to a value
of
> > > + at least 1000 ms.
> > > + </p>
> >
> > Looks like some copy/paste issue ? Because the value is a boolean :).
> >
> Oops. Thanks for catching it.
>
> > Thanks
> > Numan
> >
> >
> >
> > > + </column>
> > > +
> > > <group title="Options for configuring interconnection route
advertisement">
> > > <p>
> > > These options control how routes are advertised between OVN
> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > index 8344c7f..c98df49 100644
> > > --- a/tests/ovn-northd.at
> > > +++ b/tests/ovn-northd.at
> > > @@ -1781,3 +1781,17 @@ AT_CHECK([ovn-sbctl lflow-list | grep
"ls_out_pre_lb.*priority=100" | grep reg0
> > > ])
> > >
> > > AT_CLEANUP
> > > +
> > > +AT_SETUP([ovn -- disable check_lsp_is_up])
> > > +ovn_start
> > > +
> > > +ovn-nbctl ls-add sw0
> > > +ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1
"aa:aa:aa:aa:aa:aa 10.0.0.1"
> > > +
> > > +ovn-nbctl --wait=sb sync
> > > +AT_CHECK([ovn-sbctl lflow-list | grep arp | grep 10\.0\.0\.1], [1],
[ignore])
> > > +
> > > +ovn-nbctl --wait=sb set NB_Global . options:check_lsp_is_up=false
> > > +AT_CHECK([ovn-sbctl lflow-list | grep arp | grep 10\.0\.0\.1], [0],
[ignore])
> >
> > > +
> > > +AT_CLEANUP
> > > --
> > > 2.1.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
More information about the dev
mailing list