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

[Mark Michelson]

On Mon, Nov 20, 2017 at 11:48 AM Mark Michelson  wrote:

> On Tue, Nov 14, 2017 at 2:31 AM Numan Siddique 
> wrote:
>
>>
>> A few comments inline, otherwise LGTM.
>>
>> Thanks
>> Numan
>>
>>
>> On Mon, Nov 13, 2017 at 8:50 AM, 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 
>>>
>> ---
>>>  lib/packets.h|   7 +
>>>  ovn/controller/pinctrl.c | 343
>>> +++
>>>  ovn/northd/ovn-northd.c  |  66 +
>>>  ovn/ovn-nb.xml   |  19 +++
>>>  tests/ovn-northd.at  | 110 +++
>>>  tests/ovn.at | 150 +
>>>  6 files changed, 695 insertions(+)
>>>
>>> diff --git a/lib/packets.h b/lib/packets.h
>>> index 057935cbf..b8249047f 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,12 @@ BUILD_ASSERT_DECL(RA_MSG_LEN == sizeof(struct
>>> ovs_ra_msg));
>>>  #define ND_RA_MANAGED_ADDRESS 0x80
>>>  #define ND_RA_OTHER_CONFIG0x40
>>>
>>> +/* Defaults based on MaxRtrInterval and MinRtrInterval from RFC 4861
>>> section
>>> + * 6.2.1
>>> + */
>>> +#define ND_RA_MAX_INTERVAL_DEFAULT 600
>>> +#define ND_RA_MIN_INTERVAL_DEFAULT(max) ((max) >= 9 ? (max) / 3 : (max)
>>> * 3 / 4)
>>> +
>>>  /*
>>>   * 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..4e6a289b4 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,342 @@ 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;
>>> +};
>>>
>>
>> We probably don't need this structure if we reuse the function
>> parse_and_store_addresses from ovn-util.c
>> Please see below for more comments on this.
>>
>
> This was a good find, thanks! As it turns out, I did not make
> parse_and_store_addresses() public. I just used the already-public
> extract_ip_addresses() function from ovn-util.h.
>
>
>>
>> +
>>> +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;

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

[Numan Siddique]

A few comments inline, otherwise LGTM.

Thanks
Numan


On Mon, Nov 13, 2017 at 8:50 AM, 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 
>
---
>  lib/packets.h|   7 +
>  ovn/controller/pinctrl.c | 343 ++
> +
>  ovn/northd/ovn-northd.c  |  66 +
>  ovn/ovn-nb.xml   |  19 +++
>  tests/ovn-northd.at  | 110 +++
>  tests/ovn.at | 150 +
>  6 files changed, 695 insertions(+)
>
> diff --git a/lib/packets.h b/lib/packets.h
> index 057935cbf..b8249047f 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,12 @@ BUILD_ASSERT_DECL(RA_MSG_LEN == sizeof(struct
> ovs_ra_msg));
>  #define ND_RA_MANAGED_ADDRESS 0x80
>  #define ND_RA_OTHER_CONFIG0x40
>
> +/* Defaults based on MaxRtrInterval and MinRtrInterval from RFC 4861
> section
> + * 6.2.1
> + */
> +#define ND_RA_MAX_INTERVAL_DEFAULT 600
> +#define ND_RA_MIN_INTERVAL_DEFAULT(max) ((max) >= 9 ? (max) / 3 : (max)
> * 3 / 4)
> +
>  /*
>   * 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..4e6a289b4 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,342 @@ 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;
> +};
>

We probably don't need this structure if we reuse the function
parse_and_store_addresses from ovn-util.c
Please see below for more comments on this.

+
> +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; /* Managed/Other flags for RAs */
> +uint8_t la_flags; /* On-link/autonomous flags for address prefixes */
> +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;
>

small nit: can be renamed to delete_me.

> +};
> +
> +static void
> +init_ipv6_ras(void)
> +{
> +shash_init(_ras);
> +send_ipv6_ra_time = LLONG_MAX;
> +}
> +
> +static void
> +ipv6_ra_config_delete(struct 

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

[Mark Michelson]

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 
---
 lib/packets.h|   7 +
 ovn/controller/pinctrl.c | 343 +++
 ovn/northd/ovn-northd.c  |  66 +
 ovn/ovn-nb.xml   |  19 +++
 tests/ovn-northd.at  | 110 +++
 tests/ovn.at | 150 +
 6 files changed, 695 insertions(+)

diff --git a/lib/packets.h b/lib/packets.h
index 057935cbf..b8249047f 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,12 @@ BUILD_ASSERT_DECL(RA_MSG_LEN == sizeof(struct 
ovs_ra_msg));
 #define ND_RA_MANAGED_ADDRESS 0x80
 #define ND_RA_OTHER_CONFIG0x40
 
+/* Defaults based on MaxRtrInterval and MinRtrInterval from RFC 4861 section
+ * 6.2.1
+ */
+#define ND_RA_MAX_INTERVAL_DEFAULT 600
+#define ND_RA_MIN_INTERVAL_DEFAULT(max) ((max) >= 9 ? (max) / 3 : (max) * 3 / 
4)
+
 /*
  * 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..4e6a289b4 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,342 @@ 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; /* Managed/Other flags for RAs */
+uint8_t la_flags; /* On-link/autonomous flags for address prefixes */
+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(_ras);
+send_ipv6_ra_time = LLONG_MAX;
+}
+
+static void
+ipv6_ra_config_delete(struct ipv6_ra_config *config)
+{
+if (config) {
+free(config->prefixes);
+}
+free(config);
+}
+
+static void
+ipv6_ra_delete(struct ipv6_ra_state *ra)
+{
+if (ra) {
+ipv6_ra_config_delete(ra->config);
+}
+free(ra);
+}
+
+static void
+destroy_ipv6_ras(void)
+{
+struct shash_node *iter, *next;
+SHASH_FOR_EACH_SAFE (iter, next, _ras) {
+struct ipv6_ra_state *ra = iter->data;
+ipv6_ra_delete(ra);
+shash_delete(_ras, iter);
+}
+shash_destroy(_ras);
+}
+
+static struct ipv6_ra_config *
+ipv6_ra_update_config(const struct