[ovs-dev] [PATCH v6 ovn 1/2] controller: add ipv6 prefix delegation state machine

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Mon Mar 16 18:44:42 UTC 2020


> "
> 
> On Tue, Jan 14, 2020 at 4:37 PM Lorenzo Bianconi
> <lorenzo.bianconi at redhat.com> wrote:
> >
> > Introduce IPv6 Prefix delegation state machine according to RFC 3633
> > https://tools.ietf.org/html/rfc3633.
> > Add handle_dhcpv6_reply controller action to parse advertise/reply from
> > IPv6 delegation server. Advertise/reply are parsed running respectively:
> > - pinctrl_parse_dhcv6_advt
> > - pinctrl_parse_dhcv6_reply
> > The IPv6 requesting router starts sending dhcpv6 solicit through the logical
> > router port marked with ipv6_prefix_delegation set to true.
> > An IPv6 prefix will be requested for each logical router port marked
> > with "prefix" set to true in option column of logical router port table.
> > Save IPv6 prefix received by IPv6 delegation router in the options columns of
> > SB port binding table in order to be reused by Router Advertisement framework
> > run by ovn logical router pipeline.
> > IPv6 Prefix delegation state machine is enabled on Gateway Router or on
> > a Gateway Router Port
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> 
> 
> Hi Lorenzo,
> Thanks for the v6.

Hi Numan,

thx for the reivew and sorry for the delay

> 
> I tested this series.  Below are some comments
> 
>  - When I configured the prefix options on the router ports,
> ovn-controller hosting the gateway
>    router port sends the PD solicit message and gets the prefix, but
> it never sends the renew/Solicit
>    requests periodically. In the code I see that the next announce
> time is chosen. But looks like that is not
>   working. I think it's better to use the "prefix lifetime option"
> value sent by the delegating server rather than
>   current_time + random(3000) which this patch does.

renew message timeout is set based on t1 field of IA_PD option received
from the delation router. According to the RFC3633 (pag 8):

"The time at which the requesting router should contact the delegating router
 from which the prefixes in the IA_PD were obtained to extend the lifetimes
 of the prefixes delegated to the IA_PD"

Do you think we should use preferred-lifetime/valid-lifetime fields?
> 
> - Suppose if a router port had received a prefix 'P1' and if I restart
> ovn-controller, ovn-controller
>   sends the PD messages and gets another prefix 'P2'. Ideally
> ovn-controller should try to renew
>   the existing prefix. Can you please check if it is possible to
> include the prefix option in the Request message ?
>   Probably ovn-controller can send a Renew request if a router port
> has already a prefix ? If the delegating server,
>   can't allocate the same prefix, ovn-controller can start the fresh
> process of sending Solicit message.

ack, I will fix it in v7

> 
> - In the above case, I notice that when delegating server sends 'P2',
> ovn-controller doesn't update this new prefix
>   in the port_binding.options:ipv6_ra_pd_list. And the patch 2 of this
> series, doesn't update the "ipv6_prefix column
>   of logical router port.  I think it's better for ovn-northd to just
> update/reset the "ipv6_prefix" column from the
>   port_binding.options:ipv6_ra_pd_list, rather than appending the value.

ack, I will fix it in v7

> 
>  - The action handle_dhcp6_reply takes inner actions. But this is not
> documented.
>    I think it's better not to include the inner actions. When the
> delegating server sends the "reply" message, ovn-controller
>    doesn't need to send any reply. But since this action is used for
> handling the "Advertise" and "Reply" messages from
>    the delegating router, it doesn't seem appropriate to include the
> inner actions.
> 
> Please see some more comments below
> 
> 
> > ---
> >  controller/ovn-controller.c |   1 +
> >  controller/pinctrl.c        | 612 +++++++++++++++++++++++++++++++++++-
> >  controller/pinctrl.h        |   2 +
> >  include/ovn/actions.h       |   8 +-
> >  lib/actions.c               |  22 ++
> >  lib/ovn-l7.h                |  19 ++
> >  ovn-sb.xml                  |   8 +
> >  tests/ovn.at                |   6 +
> >  utilities/ovn-trace.c       |   3 +
> >  9 files changed, 673 insertions(+), 8 deletions(-)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 17744d416..d559e845e 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2145,6 +2145,7 @@ main(int argc, char *argv[])
> >                      runtime_data = engine_get_data(&en_runtime_data);
> >                      if (runtime_data) {
> >                          pinctrl_run(ovnsb_idl_txn,
> > +                                    ovnsb_idl_loop.idl,
> >                                      sbrec_datapath_binding_by_key,
> >                                      sbrec_port_binding_by_datapath,
> >                                      sbrec_port_binding_by_key,

[...]

> > +pinctrl_prefixd_state_handler(const struct flow *ip_flow,
> > +                              struct in6_addr addr, unsigned aid,
> > +                              char prefix_len, unsigned t1, unsigned t2,
> > +                              unsigned plife_time, unsigned vlife_time)
> > +{
> > +    struct ipv6_prefixd_state *pfd;
> > +
> > +    pfd = pinctrl_find_prefixd_state(ip_flow, aid);
> > +    if (pfd) {
> > +        pfd->state = PREFIX_PENDING;
> > +        pfd->plife_time = plife_time;
> > +        pfd->vlife_time = vlife_time;
> 
> We don't make use of these values at all ? We probably need to make
> use of preferred life time atleast ? and
> send renewal just before this value expires ?

we are not using them for the moment, do you prefer to remove them?

> 
> 
> > +        pfd->plen = prefix_len;
> > +        pfd->prefix = addr;
> > +        pfd->t1 = t1;
> > +        pfd->t2 = t2;
> > +        notify_pinctrl_main();
> > +    }
> > +}
> > +
> > +static void
> > +pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in,

[...]

> > +    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> > +        const struct sbrec_port_binding *pb, *pb_next;
> > +        SBREC_PORT_BINDING_FOR_EACH_SAFE (pb, pb_next, ovnsb_idl) {
> 
> We are having totally 4 for loops. 2 here and 2 in the fill_ipv6_prefix_state().
> In a highly loaded system, this can be a bottleneck. I think we can
> optimize here.
> 
> something like
> 
> 
> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
>     if (!smap_get(&ld->datapath->external_ids, "logical-router") {
>         /* This is a logical switch and we can skip it. */
>         continue;
>     }
> 
>    for (size_t i = 0; i < ld->n_peer_ports; ld++) {
>           if (!smap_get_bool(&ld->peer[i]->options,
> "ipv6_prefix_delegation", false)) {
>                continue;
>           }
> 
>           ....
>            ....
>    }
> }

ack, I will fix it in v7

Regards,
Lorenzo

> 
> 
> > +            if (!smap_get_bool(&pb->options, "ipv6_prefix_delegation",
> > +                               false)) {
> > +                continue;
> > +            }
> > +
> > +            const char *peer_s = smap_get(&pb->options, "peer");
> > +            if (!peer_s) {
> > +                continue;
> > +            }
> > +
> > +            const struct sbrec_port_binding *peer
> > +                = lport_lookup_by_name(sbrec_port_binding_by_name, peer_s);
> > +            if (!peer) {
> > +                continue;
> > +            }
> > +
> > +            char *redirect_name = xasprintf("cr-%s", pb->logical_port);
> > +            bool resident = lport_is_chassis_resident(
> > +                    sbrec_port_binding_by_name, chassis, active_tunnels,
> > +                    redirect_name);
> > +            free(redirect_name);
> > +            if (!resident && strcmp(pb->type, "l3gateway")) {
> > +                continue;
> > +            }
> > +
> > +            struct in6_addr ip6_addr;
> > +            struct eth_addr ea;
> > +            for (i = 0; i < pb->n_mac; i++) {
> > +                struct lport_addresses laddrs;
> > +
> > +                if (!extract_lsp_addresses(pb->mac[i], &laddrs)) {
> > +                    continue;
> > +                }
> > +
> > +                ea = laddrs.ea;
> > +                if (laddrs.n_ipv6_addrs > 0) {
> > +                    ip6_addr = laddrs.ipv6_addrs[0].addr;
> > +                    break;
> > +                }
> > +            }
> > +
> > +            if (eth_addr_is_zero(ea)) {
> > +                continue;
> > +            }
> > +
> > +            if (i == pb->n_mac) {
> > +                in6_generate_lla(ea, &ip6_addr);
> > +            }
> > +
> > +            changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, ovnsb_idl,
> > +                                              local_datapaths, ea, ip6_addr,
> > +                                              peer->tunnel_key,
> > +                                              peer->datapath->tunnel_key);
> > +        }
> > +    }
> > +
> > +    if (changed) {
> > +        notify_pinctrl_handler();
> > +    }
> > +}
> > +
> >  /* Called by pinctrl_run(). Runs with in the main ovn-controller
> >   * thread context. */
> >  void
> > @@ -2743,6 +3339,7 @@ pinctrl_destroy(void)
> >      free(pinctrl.br_int_name);
> >      destroy_send_garps_rarps();
> >      destroy_ipv6_ras();
> > +    destroy_ipv6_prefixd();
> >      destroy_buffered_packets_map();
> >      event_table_destroy();
> >      destroy_put_mac_bindings();
> > @@ -4240,6 +4837,7 @@ may_inject_pkts(void)
> >  {
> >      return (!shash_is_empty(&ipv6_ras) ||
> >              !shash_is_empty(&send_garp_rarp_data) ||
> > +            ipv6_prefixd_should_inject() ||
> >              !ovs_list_is_empty(&mcast_query_list) ||
> >              !ovs_list_is_empty(&buffered_mac_bindings));
> >  }
> > diff --git a/controller/pinctrl.h b/controller/pinctrl.h
> > index 8fa4baae9..a34749f02 100644
> > --- a/controller/pinctrl.h
> > +++ b/controller/pinctrl.h
> > @@ -26,6 +26,7 @@ struct hmap;
> >  struct lport_index;
> >  struct ovsdb_idl_index;
> >  struct ovsdb_idl_txn;
> > +struct ovsdb_idl;
> >  struct ovsrec_bridge;
> >  struct sbrec_chassis;
> >  struct sbrec_dns_table;
> > @@ -34,6 +35,7 @@ struct sbrec_service_monitor_table;
> >
> >  void pinctrl_init(void);
> >  void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > +                 struct ovsdb_idl *ovnsb_idl,
> >                   struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> >                   struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
> >                   struct ovsdb_idl_index *sbrec_port_binding_by_key,
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 047a8d737..c3d719985 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -89,7 +89,8 @@ struct ovn_extend_table;
> >      OVNACT(CHECK_PKT_LARGER,  ovnact_check_pkt_larger) \
> >      OVNACT(TRIGGER_EVENT,     ovnact_controller_event) \
> >      OVNACT(BIND_VPORT,        ovnact_bind_vport)       \
> > -    OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check)
> > +    OVNACT(HANDLE_SVC_CHECK,  ovnact_handle_svc_check) \
> > +    OVNACT(DHCP6_REPLY,       ovnact_nest)
> >
> >  /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
> >  enum OVS_PACKED_ENUM ovnact_type {
> > @@ -552,6 +553,11 @@ enum action_opcode {
> >       *     MFF_LOG_INPORT = port
> >       */
> >      ACTION_OPCODE_HANDLE_SVC_CHECK,
> > +    /* handle_dhcpv6_reply { ...actions ...}."
> > +     *
> > +     *  The actions, in OpenFlow 1.3 format, follow the action_header.
> > +     */
> > +    ACTION_OPCODE_DHCP6_SERVER,
> >  };
> >
> >  /* Header. */
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 051e6c875..b7bd219e4 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -2172,6 +2172,26 @@ ovnact_put_opts_free(struct ovnact_put_opts *pdo)
> >      free_gen_options(pdo->options, pdo->n_options);
> >  }
> >
> > +static void
> > +parse_DHCP6_REPLY(struct action_context *ctx)
> > +{
> > +    parse_nested_action(ctx, OVNACT_DHCP6_REPLY, "ip6");
> > +}
> > +
> > +static void
> > +format_DHCP6_REPLY(const struct ovnact_nest *nest, struct ds *s)
> > +{
> > +    format_nested_action(nest, "handle_dhcpv6_reply", s);
> > +}
> > +
> > +static void
> > +encode_DHCP6_REPLY(const struct ovnact_nest *on,
> > +                   const struct ovnact_encode_params *ep,
> > +                   struct ofpbuf *ofpacts)
> > +{
> > +    encode_nested_actions(on, ep, ACTION_OPCODE_DHCP6_SERVER, ofpacts);
> > +}
> > +
> >  static void
> >  parse_SET_QUEUE(struct action_context *ctx)
> >  {
> > @@ -2973,6 +2993,8 @@ parse_action(struct action_context *ctx)
> >          parse_bind_vport(ctx);
> >      } else if (lexer_match_id(ctx->lexer, "handle_svc_check")) {
> >          parse_handle_svc_check(ctx);
> > +    } else if (lexer_match_id(ctx->lexer, "handle_dhcpv6_reply")) {
> > +        parse_DHCP6_REPLY(ctx);
> >      } else {
> >          lexer_syntax_error(ctx->lexer, "expecting action");
> >      }
> > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
> > index 375b77014..2987277a9 100644
> > --- a/lib/ovn-l7.h
> > +++ b/lib/ovn-l7.h
> > @@ -174,8 +174,11 @@ struct dhcp_opt6_header {
> >  #define DHCPV6_OPT_SERVER_ID_CODE        2
> >  #define DHCPV6_OPT_IA_NA_CODE            3
> >  #define DHCPV6_OPT_IA_ADDR_CODE          5
> > +#define DHCPV6_OPT_STATUS_CODE           13
> >  #define DHCPV6_OPT_DNS_SERVER_CODE       23
> >  #define DHCPV6_OPT_DOMAIN_SEARCH_CODE    24
> > +#define DHCPV6_OPT_IA_PD                 25
> > +#define DHCPV6_OPT_IA_PREFIX             26
> >
> >  #define DHCPV6_OPT_SERVER_ID \
> >      DHCP_OPTION("server_id", DHCPV6_OPT_SERVER_ID_CODE, "mac")
> > @@ -254,6 +257,22 @@ struct ovs_nd_route_info {
> >  };
> >  BUILD_ASSERT_DECL(ND_ROUTE_INFO_OPT_LEN == sizeof(struct ovs_nd_route_info));
> >
> > +OVS_PACKED(
> > +struct dhcpv6_opt_ia_prefix {
> > +    struct dhcpv6_opt_header opt;
> > +    ovs_be32 plife_time;
> > +    ovs_be32 vlife_time;
> > +    uint8_t plen;
> > +    struct in6_addr ipv6;
> > +});
> > +
> > +OVS_PACKED(
> > +struct dhcpv6_opt_status {
> > +    struct dhcpv6_opt_header opt;
> > +    ovs_be16 status_code;
> > +    uint8_t msg[];
> > +});
> > +
> >  #define DHCPV6_DUID_LL      3
> >  #define DHCPV6_HW_TYPE_ETH  1
> >
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index 82167c488..14fe249ec 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -2114,6 +2114,14 @@ tcp.flags = RST;
> >
> >            <p><b>Example:</b> <code>handle_svc_check(inport);</code></p>
> >          </dd>
> > +
> > +        <dt><code>handle_dhcpv6_reply;</code></dt>
> > +        <dd>
> > +          <p>
> > +            This action is used to parse DHCPv6 replies from IPv6
> > +            Delegation Router and managed IPv6 Prefix delegation state machine
> > +          </p>
> > +        </dd>
> >        </dl>
> >      </column>
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 411b76804..9aa30d435 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1481,6 +1481,12 @@ handle_svc_check();
> >  handle_svc_check(reg0);
> >      Cannot use numeric field reg0 where string field is required.
> >
> > +# prefix delegation
> > +handle_dhcpv6_reply{};
> > +    formats as handle_dhcpv6_reply { drop; };
> > +    encodes as controller(userdata=00.00.00.13.00.00.00.00)
> > +    has prereqs ip6
> > +
> >  # Miscellaneous negative tests.
> >  ;
> >      Syntax error at `;'.
> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > index 264543876..3b132a2eb 100644
> > --- a/utilities/ovn-trace.c
> > +++ b/utilities/ovn-trace.c
> > @@ -2224,6 +2224,9 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> >
> >          case OVNACT_HANDLE_SVC_CHECK:
> >              break;
> > +
> > +        case OVNACT_DHCP6_REPLY:
> > +            break;
> >          }
> >      }
> >      ds_destroy(&s);
> > --
> > 2.21.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> 


More information about the dev mailing list