On Tue, Sep 8, 2020 at 11:27 AM Han Zhou <[email protected]> wrote: > > > > On Tue, Sep 8, 2020 at 5:29 AM Numan Siddique <[email protected]> wrote: > > > > On Thu, Sep 3, 2020 at 7:44 AM Han Zhou <[email protected]> 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 <[email protected]> > > > > > > Few minor comments. > > > > Acked-by: Numan Siddique <[email protected]> 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/[email protected]/ 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 > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
