[ovs-dev] [PATCH v2 RFC] ovn: Support native dhcp using 'continuations'

[Numan Siddique]

On Fri, Apr 22, 2016 at 10:06 PM, Ben Pfaff <blp at ovn.org> wrote:

> On Mon, Apr 18, 2016 at 10:59:16PM +0530, Numan Siddique wrote:
> > 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 <nusiddiq at redhat.com>
>
> Thanks for working on this!
>
> I get one error:
>
>         ../ovn/lib/actions.c:355:34: error: implicit declaration of
> function 'ip_count_cidr_bits' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
>
>
​Done. Fixed it.

​


> All the casts in dhcp.h are unusual, any particular reason for them?
>
> ​​
> This seems pretty darned well put together for an RFC!  I'll look
> forward to the final version.
>
> There are some oddities in the documentation: <p> and <dl> shouldn't
> normally nest in <p>, and usually one would terminate the <p> between
> paragraphs (probably the documentation processor should be pickier):
>
> +        <p>
> +          OVN implements a native DHCP support which caters to the common
> +          use case of providing an IP address to a booting instance by
> providing
> +          stateless replies to DHCP requests based on statically
> configured
> +          address mappings. To do this it allows a short list of DHCP
> options to be
> +          configured and applied at each compute host running
> ovn-controller.
> +
> +          This column is a key/value pair with key being the
> <code>cidr</code>
> +          of the subnet belonging to this logical switch and value being
> the
> +          <code>DHCP options</code> defined as a valid JSON string with
> +          key/value pairs as DHCP options. All the values should be
> strings.
> +
> +          <p>
> +            Examples:
> +          </p>
>
> I'm not sure there's much value in adding a specialized ovn-nbctl
> command, though it's harmless.
>
>

​​I have missed addressing these comments in the v3. I will ​address them
in v4 along with your comments once you get a chance to review.

Thanks
Numan
​