[ovs-dev] [PATCH v2 2/2] ovn-northd ipam: Support IPv6 dynamic assignment

[Numan Siddique]

Thanks for the review.


On Thu, Mar 9, 2017 at 2:48 AM, Ben Pfaff <blp at ovn.org> wrote:

> On Tue, Mar 07, 2017 at 07:19:45AM +0530, nusiddiq at redhat.com wrote:
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > OVN will generate the IPv6 address for a logical port if requested
> > using the IPv6 prefix and the MAC address (as IEEE EUI64 identifier).
> > To generate the IPv6 address, CMS should define the IPv6 prefix in the
> > 'Logical_switch.other_config:ipv6_prefix' column.
> >
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
>
> The documentation uses <dl> for a bulleted list.  Please use <ul>.
>
>
​Done.
​


> The test uses explicit "sleep" calls.  Are these needed?  I guess that
> --wait=hv would also work, but faster.
>
> ​
It was a mistake. WIll remove it.

​


> I found ipam_allocate_addresses() to be a little hard to follow.  How
> about rewriting it like this?  I have not tested it beyond compiling,
> but the general code flow seems more straightforward to me.
>
> static bool
> ipam_allocate_addresses(struct ovn_datapath *od, struct ovn_port *op,
>                         const char *addrspec)
> {
>     if (!op->nbsp || !od->ipam_info) {
>         return false;
>     }
>
>     /* Get or generate MAC address. */
>     struct eth_addr mac;
>     bool dynamic_mac;
>     int n = 0;
>     if (ovs_scan(addrspec, ETH_ADDR_SCAN_FMT" dynamic%n",
>                  ETH_ADDR_SCAN_ARGS(mac), &n)
>         && addrspec[n] == '\0') {
>         dynamic_mac = false;
>     } else {
>         uint64_t mac64 = ipam_get_unused_mac();
>         if (!mac64) {
>             return false;
>         }
>         eth_addr_from_uint64(mac64, &mac);
>         dynamic_mac = true;
>     }
>
>     /* Generate IPv4 address, if desirable. */
>     bool dynamic_ip4 = od->ipam_info->allocated_ipv4s != NULL;
>     uint32_t ip4 = dynamic_ip4 ? ipam_get_unused_ip(od) : 0;
>
>     /* Generate IPv6 address, if desirable. */
>     bool dynamic_ip6 = od->ipam_info->ipv6_prefix_set;
>     struct in6_addr ip6;
>     if (dynamic_ip6) {
>         in6_generate_eui64(mac, &od->ipam_info->ipv6_prefix, &ip6);
>     }
>
>     /* If we didn't generate anything, bail out. */
>     if (!dynamic_ip4 && !dynamic_ip6) {
>         return false;
>     }
>
>     /* Save the dynamic addresses. */
>     struct ds new_addr = DS_EMPTY_INITIALIZER;
>     ds_put_format(&new_addr, ETH_ADDR_FMT, ETH_ADDR_ARGS(mac));
>     if (dynamic_ip4) {
>         ipam_insert_ip(od, ip4);
>         ds_put_format(&new_addr, " "IP_FMT, IP_ARGS(htonl(ip4)));
>     }
>     if (dynamic_ip6) {
>         char ip6_s[INET6_ADDRSTRLEN + 1];
>         ipv6_string_mapped(ip6_s, &ip6);
>         ds_put_format(&new_addr, " %s", ip6_s);
>     }
>     ipam_insert_mac(&mac, !dynamic_mac);
>     nbrec_logical_switch_port_set_dynamic_addresses(op->nbsp,
>                                                     ds_cstr(&new_addr));
>     ds_destroy(&new_addr);
>     return true;
> }
>

​Thanks for making it cleaner.
​


>
> I am a little concerned that, when no dynamic address is assigned, the
> code does not clear the dynamic_addresses column.  That means that, if
> "dynamic" is removed from the addresses column, the former dynamic
> addresses will still appear.  I suspect that the code should clear
> dynamic_addresses when there are no dynamic addresses.
>

​
I have addressed this in the v3 patch 1.
​

>
> Thanks,
>
> Ben.
>