On Mon, Apr 4, 2016 at 8:10 PM, Ryan Moats <rmo...@us.ibm.com> wrote:
> Russell Bryant <russ...@ovn.org> wrote on 04/04/2016 09:27:30 AM:
>
> > From: Russell Bryant <russ...@ovn.org>
> > To: Ryan Moats/Omaha/IBM@IBMUS
> > Cc: Numan Siddique <nusid...@redhat.com>, ovs dev <dev@openvswitch.org>
> > Date: 04/04/2016 09:28 AM
> > Subject: Re: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp
> > using'continuations'
>
> >
> > On Mon, Apr 4, 2016 at 8:46 AM, Ryan Moats <rmo...@us.ibm.com> wrote:
> >
> > "dev" <dev-boun...@openvswitch.org> wrote on 04/04/2016 06:41:01 AM:
> >
> > > From: Numan Siddique <nusid...@redhat.com>
> > > To: ovs dev <dev@openvswitch.org>
> > > Date: 04/04/2016 06:41 AM
> > > Subject: [ovs-dev] [PATCH v1 RFC] ovn: Support native dhcp using
> > > 'continuations'
> > > Sent by: "dev" <dev-boun...@openvswitch.org>
> > >
> > > To support native dhcp in ovn
> > > - A new column 'dhcp-options' is added in 'Logical_Switch' north db.
> > > - A logical flow is added for each logical port to handle dhcp packets
> > > if the CMS has defined dhcp options in this column.
> > >
> > > Eg.
> > > action=(dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1,
> > > server_id = 10.0.0.2, mtu = 1300, lease_time = 3600,
> > > netmask = 255.255.255.0); eth.dst = eth.src; eth.src =
> 00:00:00:00:00:03;
> > > ip4.dst = 10.0.0.2; ip4.src = 10.0.0.2; udp.src = 67; udp.dst = 68;
> > > outport = inport; inport = ""; /* Allow sending out inport. */ output;)
> > >
> > > - ovn-controller will convert this logical flow to a packet-in OF flow
> > with
> > > 'pause' flag set. The dhcp options defined in 'dhcp_offer' action
> > > are stored in the 'userdata'
> > >
> > > - When the ovn-controller receives the packet-in, it would frame a new
> > > dhcp packet with the dhcp options set in the 'userdata' and resume
> > > the packet.
> > >
> > > TODO: Test cases and updating the necessary documentation.
> > >
> > > Signed-off-by: Numan Siddique <nusid...@redhat.com>
> > > ---
> >
> > [snip]
> >
> > First, let me say that I'm a big fan of this idea in general...
> >
> > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> > > index bcad318..a175458 100644
> > > --- a/ovn/controller/lflow.c
> > > +++ b/ovn/controller/lflow.c
> > > @@ -28,6 +28,7 @@
> > > #include "packets.h"
> > > #include "simap.h"
> > >
> > > +
> > > VLOG_DEFINE_THIS_MODULE(lflow);
> > >
> > > /* Symbol table. */
> > > @@ -35,6 +36,9 @@ VLOG_DEFINE_THIS_MODULE(lflow);
> > > /* Contains "struct expr_symbol"s for fields supported by OVN lflows.
> */
> > > static struct shash symtab;
> > >
> > > +/* Contains "struct expr_symbol_dhcp_opts"s for dhcp options */
> > > +static struct shash dhcp_opt_symtab;
> > > +
> > > static void
> > > add_logical_register(struct shash *symtab, enum mf_field_id id)
> > > {
> > > @@ -156,6 +160,27 @@ lflow_init(void)
> > > expr_symtab_add_predicate(&symtab, "sctp", "ip.proto == 132");
> > > expr_symtab_add_field(&symtab, "sctp.src", MFF_SCTP_SRC, "sctp",
> > false);
> > > expr_symtab_add_field(&symtab, "sctp.dst", MFF_SCTP_DST, "sctp",
> > false);
> > > +
> > > + /* dhcp options */
> > > + shash_init(&dhcp_opt_symtab);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "offerip", 0,
> > > + DHCP_OPT_TYPE_IP4);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "netmask", 1,
> > > + DHCP_OPT_TYPE_IP4);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "router", 3,
> > > + DHCP_OPT_TYPE_IP4);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "dns_server", 6,
> > > + DHCP_OPT_TYPE_IP4);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "domain_name",
> 15,
> > > + DHCP_OPT_TYPE_STR);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab,
> > "ip_forward_enable", 19,
> > > + DHCP_OPT_TYPE_BOOL);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "mtu", 26,
> > > + DHCP_OPT_TYPE_UINT16);
> > > + dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "lease_time", 51,
> > > + DHCP_OPT_TYPE_UINT32);
> > > +
>
> dhcp_opt_expr_symtab_add_field(&dhcp_opt_symtab, "server_id", 54,
> > > + DHCP_OPT_TYPE_IP4);
>
> > but I'm rather uncomfortable with (a) using the bare option numbers
> > above (I think I'd rather see symbols and values defined in dhcp.h)
> > and (b) not supporting more/all of RFC 2132 (under this heading, isn't
> > opt 0 padding, rather than the offerip?)
> >
>
Thanks Ryan for pointing this out. I will define them in dhcp.h
'offerip' is actually not a dhcp option. I am using opt 0 since CMS would
not define
this dhcp option.
The userdata would be of the format
---------------------------------------------------------------------------------------
|ACTION_OPCODE_DHCP_OFFER (8 bytes) | offer_ip (4 bytes) | <dhcp options>
|
--------------------------------------------------------------------------------------
> > I agree that some constants would be an improvement.
> >
> > Maybe I'm misreading your suggestion, but I definitely do not think
> > we should be trying to implement a "full featured" DHCP server in
> > OVN. The intention here is "just enough" DHCP. We only need
> > minimal support to be able to issue DHCP responses based on
> > statically defined IP/MAC mappings.
>
> I certainly don't want a full DHCP server in OVN and so I'd like to
> see something somewhere that *says* this isn't a full featured DHCP
> as it provides some minimal set of functionality (and then listing
> the functions). However, my sad experience with providing a minimal
> set of functionality is that it becomes a slippery slope of "just
> one more thing" (for example, what about static routes?).
>
Even I was thinking to identify a minimal set of dhcp options and support
them.
If the requirement arises for more dhcp options, we can raise a bug. In
order to support
more dhcp options we just need to define them in 'dhcp_opt_symtab'.
In the next version, I will add more dhcp options which are more likely to
be used, although I am not sure how to select them :)
Thanks
Numan
>
> Ryan
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
