[ovs-dev,v4] ovn-northd, tests: Adding IPAM to ovn-northd.
diff mbox

Message ID CAM_3v9+PG2CXHtG=sxE4E6dE=sPTeKZgN+fLy7KAG_KCQSSNRw@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Guru Shetty July 28, 2016, 8:29 p.m. UTC
On 28 July 2016 at 11:37, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Jul 27, 2016 at 11:28:24AM -0700, Nimay Desai wrote:
> > Added an IPv4 and MAC addresses management system to ovn-northd. When a
> logical
> > switch's other_config:subnet field is set, logical ports attached to that
> > switch that have the keyword "dynamic" in their addresses column will
> > automatically be allocated a globally unique MAC address/unused IPv4
> address
> > within the provided subnet. The allocated address will populate the
> > dynamic_addresses column. This can be useful for a user who wants to
> deploy
> > many VM's or containers with networking capabilities, but does not care
> about
> > the specific MAC/IPv4 addresses that are assigned.
> >
> > Added tests in ovn.at for ipam.
> >
> > Signed-off-by: Nimay Desai <nimaydesai1@gmail.com>
>
> The code uses the abbreviations 'ipam' and 'macam' a lot.  IPAM is a
> fairly common term but I don't know what macam stands for.  I think it
> would be a good idea to explain both terms in comments.
>

Do you think this incremental is helpful?

 /* Pipeline stages. */
@@ -879,6 +879,12 @@ static void
 build_ipam(struct northd_context *ctx, struct hmap *datapaths,
            struct hmap *ports)
 {
+    /* IPAM generally stands for IP address management.  In non-virtualized
+     * world, MAC addresses come with the hardware.  But, with virtualized
+     * workloads, they need to be assigned and managed.  This function
+     * does both IP address management (ipam) and MAC address management
+     * (macam). */
+
     if (!ctx->ovnnb_txn) {
         return;
     }


>
> Here are some suggested improvements.  The code improvements are, I
> hope, self-explanatory.  The changes to the documentation are mainly to
> make it clear that ovn-northd does the work.  (I find it really
> confusing when documentation says that something happens, but not what
> does it, because then you have to assume or guess what does it and it's
> easy to guess wrong.)
>
> I'm done with review.  I'll leave it to Guru to shepherd this in though
> since he's been guiding the work.
>
> Acked-by: Ben Pfaff <blp@ovn.org>
>
> --8<--------------------------cut here-------------------------->8--
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 25af707..d5cbd37 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -867,21 +867,9 @@ ipam_allocate_addresses(struct ovn_datapath *od,
> struct ovn_port *op,
>      ipam_insert_ip(od, ip, false);
>      ipam_insert_mac(&mac, false);
>
> -    /* Convert IP to string form. */
> -    struct ds ip_ds;
> -    ds_init(&ip_ds);
> -    ds_put_format(&ip_ds, IP_FMT, IP_ARGS(htonl(ip)));
> -
> -    /* Convert MAC to string form. */
> -    struct ds mac_ds;
> -    ds_init(&mac_ds);
> -    ds_put_format(&mac_ds, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
> -
> -    char *new_addr = xasprintf("%s %s", mac_ds.string, ip_ds.string);
> -    nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp,
> -                                                    (const char*)
> new_addr);
> -    ds_destroy(&ip_ds);
> -    ds_destroy(&mac_ds);
> +    char *new_addr = xasprintf(IP_FMT" "ETH_ADDR_FMT,
> +                               IP_ARGS(htonl(ip)), ETH_ADDR_ARGS(mac));
> +    nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp, new_addr);
>      free(new_addr);
>
>      return true;
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 249d3c5..85aa2d3 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -125,11 +125,12 @@
>        </p>
>
>        <column name="other_config" key="subnet">
> -        If set, logical ports that are attached to this switch that have
> the
> -        "dynamic" keyword in their addresses column will automatically be
> -        allocated a globally unique MAC address/unused IPv4 address
> within the
> -        provided IPv4 subnet.  The allocated address will populate the
> -        <ref column="dynamic_addresses"/> column.
> +        Set this to an IPv4 subnet, e.g. <code>192.168.0.0/24</code>, to
> enable
> +        <code>ovn-northd</code> to automatically assign IP addresses
> within
> +        that subnet.  Use the <code>dynamic</code> keyword in the <ref
> +        table="Logical_Switch_Port"/> table's <ref
> table="Logical_Switch_Port"
> +        column="addresses"/> column to request dynamic address assignment
> for a
> +        particular port.
>        </column>
>      </group>
>
> @@ -439,22 +440,23 @@
>
>            <dt><code>dynamic</code></dt>
>            <dd>
> -            This indicates that the logical port should be automatically
> -            assigned a globally unique MAC address and an unused IPv4
> address
> -            within the subnet that this logical port belongs to.  The
> assigned
> -            addresses will populate the <ref column="dynamic_addresses"/>
> -            column.  For this keyword to work properly, the
> other_config:subnet
> -            of the logical switch that this logical port is attached to
> must be
> -            set.
> +            Use this keyword to make <code>ovn-northd</code> generate a
> +            globally unique MAC address and choose an unused IPv4 address
> with
> +            the logical port's subnet and store them in the port's <ref
> +            column="dynamic_addresses"/> column.  <code>ovn-northd</code>
> will
> +            use the subnet specified in <ref table="Logical_Switch"
> +            column="other_config" key="subnet"/> in the port's <ref
> +            table="Logical_Switch"/>.
>            </dd>
>          </dl>
>        </column>
>
>        <column name="dynamic_addresses">
>          <p>
> -          Addresses assigned to the logical port by the IPAM.  Addresses
> will
> -          be of the same format as those that populate the
> -          <ref column="addresses"/> column.  Note that these addresses are
> +          Addresses assigned to the logical port by
> <code>ovn-northd</code>, if
> +          <code>dynamic</code> is specified in <ref column="addresses"/>.
> +          Addresses will be of the same format as those that populate the
> <ref
> +          column="addresses"/> column.  Note that these addresses are
>            constructed and managed locally in ovn-northd, so they cannot be
>            reconstructed in the event that the database is lost.
>          </p>
>

Comments

Ben Pfaff July 28, 2016, 8:54 p.m. UTC | #1
On Thu, Jul 28, 2016 at 01:29:42PM -0700, Guru Shetty wrote:
> On 28 July 2016 at 11:37, Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Wed, Jul 27, 2016 at 11:28:24AM -0700, Nimay Desai wrote:
> > > Added an IPv4 and MAC addresses management system to ovn-northd. When a
> > logical
> > > switch's other_config:subnet field is set, logical ports attached to that
> > > switch that have the keyword "dynamic" in their addresses column will
> > > automatically be allocated a globally unique MAC address/unused IPv4
> > address
> > > within the provided subnet. The allocated address will populate the
> > > dynamic_addresses column. This can be useful for a user who wants to
> > deploy
> > > many VM's or containers with networking capabilities, but does not care
> > about
> > > the specific MAC/IPv4 addresses that are assigned.
> > >
> > > Added tests in ovn.at for ipam.
> > >
> > > Signed-off-by: Nimay Desai <nimaydesai1@gmail.com>
> >
> > The code uses the abbreviations 'ipam' and 'macam' a lot.  IPAM is a
> > fairly common term but I don't know what macam stands for.  I think it
> > would be a good idea to explain both terms in comments.
> >
> 
> Do you think this incremental is helpful?

Yes, thanks!

Patch
diff mbox

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 02b26bb..18756e9 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -62,8 +62,8 @@  static const char *default_sb_db(void);
 #define MAC_ADDR_PREFIX 0x0A0000000000ULL
 #define MAC_ADDR_SPACE 0xffffff

-/* MAC address table of "struct eth_addr"s, that holds the MAC addresses
- * allocated by the ipam. */
+/* MAC address management (macam) table of "struct eth_addr"s, that holds
the
+ * MAC addresses allocated by the OVN ipam module. */
 static struct hmap macam = HMAP_INITIALIZER(&macam);
 ^L