On Fri, Nov 3, 2017 at 9:54 PM, Mark Michelson <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > --- > > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
