On 28 July 2016 at 11:37, Ben Pfaff <b...@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 <nimaydes...@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 <b...@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> > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev