From patchwork Thu Jul 28 20:29:42 2016
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
X-Patchwork-Submitter: Gurucharan Shetty
X-Patchwork-Id: 653872
Return-Path:
X-Original-To: incoming@patchwork.ozlabs.org
Delivered-To: patchwork-incoming@bilbo.ozlabs.org
Received: from archives.nicira.com (archives.nicira.com [96.126.127.54])
by ozlabs.org (Postfix) with ESMTP id 3s0k4Q4kDTz9t1P
for ;
Fri, 29 Jul 2016 06:29:57 +1000 (AEST)
Received: from archives.nicira.com (localhost [127.0.0.1])
by archives.nicira.com (Postfix) with ESMTP id 512C2112F6;
Thu, 28 Jul 2016 13:29:54 -0700 (PDT)
X-Original-To: dev@openvswitch.org
Delivered-To: dev@openvswitch.org
Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67])
by archives.nicira.com (Postfix) with ESMTPS id 612E3112F5
for ; Thu, 28 Jul 2016 13:29:52 -0700 (PDT)
Received: from bar5.cudamail.com (localhost [127.0.0.1])
by mx1e3.cudamail.com (Postfix) with ESMTPS id D0BEC420407
for ; Thu, 28 Jul 2016 14:29:51 -0600 (MDT)
X-ASG-Debug-ID: 1469737789-09eadd7aea30cf10001-byXFYA
Received: from mx3-pf2.cudamail.com ([192.168.14.1]) by bar5.cudamail.com
with
ESMTP id 49xoH6P8rxNKM8SA (version=TLSv1 cipher=DHE-RSA-AES256-SHA
bits=256 verify=NO) for ;
Thu, 28 Jul 2016 14:29:49 -0600 (MDT)
X-Barracuda-Envelope-From: guru@ovn.org
X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.1
Received: from unknown (HELO relay8-d.mail.gandi.net) (217.70.183.201)
by mx3-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted);
28 Jul 2016 20:29:48 -0000
Received-SPF: pass (mx3-pf2.cudamail.com: SPF record at ovn.org designates
217.70.183.201 as permitted sender)
X-Barracuda-Apparent-Source-IP: 217.70.183.201
X-Barracuda-RBL-IP: 217.70.183.201
Received: from mfilter16-d.gandi.net (mfilter16-d.gandi.net [217.70.178.144])
by relay8-d.mail.gandi.net (Postfix) with ESMTP id 2FBC6405A2
for ; Thu, 28 Jul 2016 22:29:46 +0200 (CEST)
X-Virus-Scanned: Debian amavisd-new at mfilter16-d.gandi.net
Received: from relay8-d.mail.gandi.net ([IPv6:::ffff:217.70.183.201])
by mfilter16-d.gandi.net (mfilter16-d.gandi.net [::ffff:10.0.15.180])
(amavisd-new, port 10024)
with ESMTP id kXWklnwysooO for ;
Thu, 28 Jul 2016 22:29:44 +0200 (CEST)
X-Originating-IP: 209.85.215.42
Received: from mail-lf0-f42.google.com (mail-lf0-f42.google.com
[209.85.215.42]) (Authenticated sender: guru@ovn.org)
by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1153040496
for ; Thu, 28 Jul 2016 22:29:43 +0200 (CEST)
Received: by mail-lf0-f42.google.com with SMTP id l69so57514472lfg.1
for ; Thu, 28 Jul 2016 13:29:43 -0700 (PDT)
X-Gm-Message-State:
AEkoouvLaIsPy/j2q7Lf9tfyLVdjwE//ZegTEq9OuW54Vi4C9E2RhMcF5QdIb4Q1DZkXE5G9K+DQcBriCdgo+Q==
X-Received: by 10.25.147.193 with SMTP id v184mr13929109lfd.43.1469737783422;
Thu, 28 Jul 2016 13:29:43 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.114.182.75 with HTTP; Thu, 28 Jul 2016 13:29:42 -0700 (PDT)
In-Reply-To: <20160728183725.GW22847@ovn.org>
References: <1469644104-46077-1-git-send-email-nimaydesai1@gmail.com>
<20160728183725.GW22847@ovn.org>
X-CudaMail-Envelope-Sender: guru@ovn.org
From: Guru Shetty
Date: Thu, 28 Jul 2016 13:29:42 -0700
X-Gmail-Original-Message-ID:
Message-ID:
X-CudaMail-Whitelist-To: dev@openvswitch.org
X-CudaMail-MID: CM-V2-727045459
X-CudaMail-DTE: 072816
X-CudaMail-Originating-IP: 217.70.183.201
To: Ben Pfaff
X-ASG-Orig-Subj: [##CM-V2-727045459##]Re: [ovs-dev] [PATCH v4] ovn-northd,
tests: Adding IPAM to ovn-northd.
X-Barracuda-Connect: UNKNOWN[192.168.14.1]
X-Barracuda-Start-Time: 1469737789
X-Barracuda-Encrypted: DHE-RSA-AES256-SHA
X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi
X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?=
X-Virus-Scanned: by bsmtpd at cudamail.com
X-Barracuda-BRTS-Status: 1
X-Content-Filtered-By: Mailman/MimeDel 2.1.16
Cc: ovs dev , Nimay Desai
Subject: Re: [ovs-dev] [PATCH v4] ovn-northd,
tests: Adding IPAM to ovn-northd.
X-BeenThere: dev@openvswitch.org
X-Mailman-Version: 2.1.16
Precedence: list
List-Id:
List-Unsubscribe: ,
List-Archive:
List-Post:
List-Help:
List-Subscribe: ,
Errors-To: dev-bounces@openvswitch.org
Sender: "dev"
On 28 July 2016 at 11:37, Ben Pfaff 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
>
> 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
>
> --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 @@
>
>
>
> - 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
> - column.
> + Set this to an IPv4 subnet, e.g. 192.168.0.0/24
, to
> enable
> + ovn-northd
to automatically assign IP addresses
> within
> + that subnet. Use the dynamic
keyword in the [ + table="Logical_Switch_Port"/> table's ][ table="Logical_Switch_Port"
> + column="addresses"/> column to request dynamic address assignment
> for a
> + particular port.
> ]
>
>
> @@ -439,22 +440,23 @@
>
> dynamic
>
> - 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
> - 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 ovn-northd
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 [ + column="dynamic_addresses"/> column. ]ovn-northd
> will
> + use the subnet specified in [ + column="other_config" key="subnet"/> in the port's ][ + table="Logical_Switch"/>.
> ]
>
>
>
>
>
> - Addresses assigned to the logical port by the IPAM. Addresses
> will
> - be of the same format as those that populate the
> - column. Note that these addresses are
> + Addresses assigned to the logical port by
> ovn-northd
, if
> + dynamic
is specified in .
> + Addresses will be of the same format as those that populate the
> [ + 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.
> ]
>
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