[ovs-dev] [PATCH v4] ovn-northd, tests: Adding IPAM to ovn-northd.
Guru Shetty
guru at ovn.org
Thu Jul 28 20:29:42 UTC 2016
On 28 July 2016 at 11:37, Ben Pfaff <blp at 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 at 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?
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
/* 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 at 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>
>
More information about the dev
mailing list