[ovs-dev] [PATCH v2 2/2] OVN: Add support for periodic router advertisements.

Numan Siddique nusiddiq at redhat.com
Fri Nov 3 16:36:22 UTC 2017


On Fri, Nov 3, 2017 at 9:54 PM, Mark Michelson <mmichels at redhat.com> wrote:

> Thanks for the reviews Jakub. I've added an in-line comment below.
> Otherwise consider that there is an implicit "Will do!" on all of your
> other suggestions.
>
> On Fri, Nov 3, 2017 at 11:04 AM Jakub Sitnicki <jkbs at redhat.com> wrote:
>
> > Hi again Mark,
> >
> > A batch of nit-picks/suggestions & a question that I've collected so far
> > when reading through this patch. Please apply as you see fit.
> >
> > On Thu, Nov 02, 2017 at 08:47 PM GMT, Mark Michelson wrote:
> > > This change adds three new options to the Northbound
> > > Logical_Router_Port's ipv6_ra_configs option:
> > >
> > > * send_periodic: If set to "true", then OVN will send periodic router
> > > advertisements out of this router port.
> > > * max_interval: The maximum amount of time to wait between sending
> > > periodic router advertisements.
> > > * min_interval: The minimum amount of time to wait between sending
> > > periodic router advertisements.
> > >
> > > When send_periodic is true, then IPv6 RA configs, as well as some layer
> > > 2 and layer 3 information about the router port, are copied to the
> > > southbound database. From there, ovn-controller can use this
> information
> > > to know when to send periodic RAs and what to send in them.
> > >
> > > Because periodic RAs originate from each ovn-controller, the new
> > > keep-local flag is set on the packet so that ports don't receive an
> > > overabundance of RAs.
> > >
> > > Signed-off-by: Mark Michelson <mmichels at redhat.com>
> > > ---
> > >  lib/packets.h            |   4 +
> > >  ovn/controller/pinctrl.c | 349
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > >  ovn/northd/ovn-northd.c  |  65 +++++++++
> > >  ovn/ovn-nb.xml           |  19 +++
> > >  tests/ovn-northd.at      | 110 +++++++++++++++
> > >  tests/ovn.at             | 150 ++++++++++++++++++++
> > >  6 files changed, 697 insertions(+)
> > >
> > > diff --git a/lib/packets.h b/lib/packets.h
> > > index 057935cbf..9d69b93d0 100644
> > > --- a/lib/packets.h
> > > +++ b/lib/packets.h
> > > @@ -976,6 +976,7 @@ BUILD_ASSERT_DECL(ND_PREFIX_OPT_LEN ==
> sizeof(struct
> > ovs_nd_prefix_opt));
> > >
> > >  /* Neighbor Discovery option: MTU. */
> > >  #define ND_MTU_OPT_LEN 8
> > > +#define ND_MTU_DEFAULT 0
> > >  struct ovs_nd_mtu_opt {
> > >      uint8_t  type;      /* ND_OPT_MTU */
> > >      uint8_t  len;       /* Always 1. */
> > > @@ -1015,6 +1016,9 @@ BUILD_ASSERT_DECL(RA_MSG_LEN == sizeof(struct
> > ovs_ra_msg));
> > >  #define ND_RA_MANAGED_ADDRESS 0x80
> > >  #define ND_RA_OTHER_CONFIG    0x40
> > >
> > > +#define ND_RA_MAX_INTERVAL_DEFAULT 600
> > > +#define ND_RA_MIN_INTERVAL_DEFAULT(max) (max) >= 9 ? (max) / 3 :
> (max)
> > * 3 / 4
> > > +
> >
> > I don't understand the formula. It generates a sequence that is not
> > always increasing but takes a dip when max == 9. What is the reasoning
> > behind it?
> >
>
> It's based on RFC 4861 section 6.2.1.
>
> It would be nice if you could mention RFC 4861 in the code comments.

Thanks
Numan


>       MinRtrAdvInterval
>                      The minimum time allowed between sending
>                      unsolicited multicast Router Advertisements from
>                      the interface, in seconds.  MUST be no less than 3
>                      seconds and no greater than .75 *
>                      MaxRtrAdvInterval.
>
>                      Default: 0.33 * MaxRtrAdvInterval If
>                      MaxRtrAdvInterval >= 9 seconds; otherwise, the
>                      Default is 0.75 * MaxRtrAdvInterval.
>
> Note that this is not the exact quote from the RFC. I have altered the
> default text to include the suggested change from Errata 3154. Does that
> explain the formula better?
>
> I can add the citation to the code so that it makes more sense for someone
> skimming the code.
>
>
> >
> > Also, you probably want to put the expression in parenthesis to avoid
> > surprises in evalution like: ND_RA_MIN_INTERVAL_DEFAULT(max)*1000.
> >
> > >  /*
> > >   * Use the same struct for MLD and MLD2, naming members as the defined
> > fields in
> > >   * in the corresponding version of the protocol, though they are
> > reserved in the
> > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > > index 29b2dde0c..f97eba4d5 100644
> > > --- a/ovn/controller/pinctrl.c
> > > +++ b/ovn/controller/pinctrl.c
> > > @@ -48,6 +48,7 @@
> > >  #include "socket-util.h"
> > >  #include "timeval.h"
> > >  #include "vswitch-idl.h"
> > > +#include "lflow.h"
> > >
> > >  VLOG_DEFINE_THIS_MODULE(pinctrl);
> > >
> > > @@ -88,6 +89,11 @@ static void pinctrl_handle_put_nd_ra_opts(
> > >  static void pinctrl_handle_nd_ns(const struct flow *ip_flow,
> > >                                   const struct match *md,
> > >                                   struct ofpbuf *userdata);
> > > +static void init_ipv6_ras(void);
> > > +static void destroy_ipv6_ras(void);
> > > +static void ipv6_ra_wait(void);
> > > +static void send_ipv6_ras(const struct controller_ctx *ctx,
> > > +    struct hmap *local_datapaths);
> > >
> > >  COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
> > >
> > > @@ -98,6 +104,7 @@ pinctrl_init(void)
> > >      conn_seq_no = 0;
> > >      init_put_mac_bindings();
> > >      init_send_garps();
> > > +    init_ipv6_ras();
> > >  }
> > >
> > >  static ovs_be32
> > > @@ -1083,8 +1090,348 @@ pinctrl_run(struct controller_ctx *ctx,
> > >      run_put_mac_bindings(ctx);
> > >      send_garp_run(ctx, br_int, chassis, chassis_index,
> local_datapaths,
> > >                    active_tunnels);
> > > +    send_ipv6_ras(ctx, local_datapaths);
> > >  }
> > >
> > > +/* Table of ipv6_ra_state structures, keyed on logical port name */
> > > +static struct shash ipv6_ras;
> > > +
> > > +/* Next IPV6 RA in seconds. */
> > > +static long long int send_ipv6_ra_time;
> > > +
> > > +struct ipv6_network_prefix {
> > > +    struct in6_addr addr;
> > > +    unsigned int prefix_len;
> > > +};
> > > +
> > > +struct ipv6_ra_config {
> > > +    time_t min_interval;
> > > +    time_t max_interval;
> > > +    struct eth_addr eth_src;
> > > +    struct eth_addr eth_dst;
> > > +    struct in6_addr ipv6_src;
> > > +    struct in6_addr ipv6_dst;
> > > +    ovs_be32 mtu;
> > > +    uint8_t mo_flags;
> >
> > Maybe annotate? For example /* Managed/Other RA flags */
> >
> > > +    uint8_t la_flags;
> >
> > Maybe annotate? For example /* SLLAO flags */
> >
> > > +    struct ipv6_network_prefix *prefixes;
> > > +    int n_prefixes;
> > > +};
> > > +
> > > +struct ipv6_ra_state {
> > > +    long long int next_announce;
> > > +    struct ipv6_ra_config *config;
> > > +    int64_t port_key;
> > > +    int64_t metadata;
> > > +    bool deleteme;
> > > +};
> > > +
> > > +static void
> > > +init_ipv6_ras(void)
> > > +{
> > > +    shash_init(&ipv6_ras);
> > > +    send_ipv6_ra_time = LLONG_MAX;
> > > +}
> > > +
> > > +static void ipv6_ra_config_delete(struct ipv6_ra_config *config)
> >
> > Function return type not on a separate line.
> >
> > > +{
> > > +    if (config) {
> > > +        free(config->prefixes);
> > > +    }
> > > +    free(config);
> > > +}
> > > +
> > > +static void
> > > +ipv6_ra_delete(struct ipv6_ra_state *ra)
> > > +{
> > > +    ipv6_ra_config_delete(ra->config);
> > > +    free(ra);
> > > +}
> >
> > Would consider making the destructor NULL-friendly.
> >
> > [...]
> >
> > > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> > > index 8ad53cd7d..cddd5f6f2 100644
> > > --- a/ovn/ovn-nb.xml
> > > +++ b/ovn/ovn-nb.xml
> > > @@ -1369,6 +1369,25 @@
> > >          Per RFC 2460, the mtu value is recommended no less than 1280,
> so
> > >          any mtu value less than 1280 will be considered as no MTU
> > Option.
> > >        </column>
> > > +
> > > +      <column name="ipv6_ra_configs" key="send_periodic">
> > > +        Should the router port send periodic router advertisements? If
> > set to
> > > +        true, then this router interface will send router
> advertisements
> > > +        out periodically. The default is false.
> > > +      </column>
> > > +
> > > +      <column name="ipv6_ra_configs" key="max_interval">
> > > +        The maximum number of seconds to wait between sending periodic
> > router
> > > +        advertisements. This option has no effect if the
> > "send_periodic" value
> > > +        is set to false. The default is 600.
> > > +      </column>
> > > +
> > > +      <column name="ipv6_ra_configs" key="min_interval">
> > > +        The minimum number of seconds to wait between sending periodic
> > router
> > > +        advertisements. This option has no effect if the
> > "send_periodic" value
> > > +        is set to "false". The default is 0.33 * max_interval. If
> > max_interval
> > > +        is set to its defaul, then min_interval will be 200.
> >
> > Lost a letter there. Should read "default."
> >
> > > +      </column>
> > >      </group>
> > >
> > >      <group title="Options">
> >
> > [...]
> >
> > Thanks,
> > Jakub
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list