Please find my comments inline.

Please note that ipv6 is still a bit of a mystery to me, so I'll review
structure and style moreso than technical correctness.

2011/1/5 Nachi Ueno <email address hidden>:
> Nachi Ueno has proposed merging lp:~ntt-pf-lab/nova/ipv6-support into lp:nova.
>
> Requested reviews:
>  Nova Core (nova-core)
>
> For more details, see:
> https://code.launchpad.net/~ntt-pf-lab/nova/ipv6-support/+merge/45228
>
> OpenStack Compute (Nova) IPv4/IPv6 dual stack support
> http://wiki.openstack.org/BexarIpv6supportReadme
>
> Tested with
>  unit test
>  smoke test
>
> No conflict with current branch r 515.
> --
> https://code.launchpad.net/~ntt-pf-lab/nova/ipv6-support/+merge/45228
> You are subscribed to branch lp:nova.
>
> === modified file 'bin/nova-manage'
> --- bin/nova-manage     2010-12-28 20:11:41 +0000
> +++ bin/nova-manage     2011-01-05 11:56:16 +0000
> @@ -89,7 +89,7 @@
>  flags.DECLARE('network_size', 'nova.network.manager')
>  flags.DECLARE('vlan_start', 'nova.network.manager')
>  flags.DECLARE('vpn_start', 'nova.network.manager')
> -
> +flags.DECLARE('fixed_range_v6', 'nova.network.manager')
>
>  class VpnCommands(object):
>     """Class for managing VPNs."""
> @@ -432,11 +432,11 @@
>     """Class for managing networks."""
>
>     def create(self, fixed_range=None, num_networks=None,
> -               network_size=None, vlan_start=None, vpn_start=None):
> +               network_size=None, vlan_start=None, vpn_start=None,fixed_range_v6=None):

This line is too long. Nova is pep8 clean. You can install the pep8
utility and check for yourself.

>         """Creates fixed ips for host by range
>         arguments: [fixed_range=FLAG], [num_networks=FLAG],
>                    [network_size=FLAG], [vlan_start=FLAG],
> -                   [vpn_start=FLAG]"""
> +                   [vpn_start=FLAG],[fixed_range_v6=FLAG]"""

Missing a space (for consistency).

>         if not fixed_range:
>             fixed_range = FLAGS.fixed_range
>         if not num_networks:
> @@ -447,11 +447,15 @@
>             vlan_start = FLAGS.vlan_start
>         if not vpn_start:
>             vpn_start = FLAGS.vpn_start
> +        if not fixed_range_v6:
> +            fixed_range_v6 = FLAGS.fixed_range_v6
>         net_manager = utils.import_object(FLAGS.network_manager)
>         net_manager.create_networks(context.get_admin_context(),
>                                     fixed_range, int(num_networks),
>                                     int(network_size), int(vlan_start),
> -                                    int(vpn_start))
> +                                    int(vpn_start),fixed_range_v6)
> +
> +

Same here. Please add a space (pep8 will tell you this, too).

> === modified file 'nova/api/ec2/cloud.py'
> --- nova/api/ec2/cloud.py       2011-01-03 19:29:39 +0000
> +++ nova/api/ec2/cloud.py       2011-01-05 11:56:16 +0000
> @@ -30,7 +30,7 @@
>
>  from nova import context
>  import IPy
> -
> +import urllib
>  from nova import crypto
>  from nova import db
>  from nova import exception
> @@ -347,6 +347,7 @@
>             values['group_id'] = source_security_group['id']
>         elif cidr_ip:
>             # If this fails, it throws an exception. This is what we want.
> +            cidr_ip = urllib.unquote(cidr_ip).decode()
>             IPy.IP(cidr_ip)
>             values['cidr'] = cidr_ip
>         else:
> @@ -676,10 +677,16 @@
>                 if instance['fixed_ip']['floating_ips']:
>                     fixed = instance['fixed_ip']
>                     floating_addr = fixed['floating_ips'][0]['address']
> +                if instance['fixed_ip']['network'] and FLAGS.use_ipv6:
> +                    i['dnsNameV6'] = utils.to_global_ipv6(
> +                        instance['fixed_ip']['network']['cidr_v6'],
> +                        instance['mac_address'])
> +
>             i['privateDnsName'] = fixed_addr
>             i['publicDnsName'] = floating_addr
>             i['dnsName'] = i['publicDnsName'] or i['privateDnsName']
>             i['keyName'] = instance['key_name']
> +
>             if context.user.is_admin():
>                 i['keyName'] = '%s (%s, %s)' % (i['keyName'],
>                     instance['project_id'],
>

This concerns me quite a bit. Amazon EC2 doesn't support IPv6 and once
they do, we have no idea how that will affect their API.


> === modified file 'nova/tests/test_network.py'
> --- nova/tests/test_network.py  2010-12-23 18:31:46 +0000
> +++ nova/tests/test_network.py  2011-01-05 11:56:16 +0000
> @@ -97,6 +97,27 @@
>         self.context.project_id = self.projects[project_num].id
>         self.network.deallocate_fixed_ip(self.context, address)
>
> +    def test_private_ipv6(self):
> +        """Make sure ipv6 is OK"""
> +        if FLAGS.use_ipv6:
> +            instance_ref = self._create_instance(1)
> +            network_ref = db.project_get_network(
> +                                                 self.context,
> +                                                 self.context.project_id)
> +            address_v6 = db.instance_get_fixed_address_v6(
> +                                                          self.context,
> +                                                          instance_ref['id'])
> +            self.assertEqual(instance_ref['mac_address'],
> +                             utils.to_mac(address_v6))
> +            instance_ref2 = db.fixed_ip_get_instance_v6(
> +                                                        self.context,
> +                                                        address_v6)
> +            self.assertEqual(instance_ref['id'], instance_ref2['id'])
> +            self.assertEqual(address_v6,
> +                             utils.to_global_ipv6(
> +                                                  network_ref['cidr_v6'],
> +                                                  instance_ref['mac_address']))
> +

FLAGS don't really apply this way during unit tests. I shouldn't be
required to explicitly enable ipv6 to have the unit tests for ipv6 run.

>     def test_public_network_association(self):
>         """Makes sure that we can allocaate a public ip"""
>         # TODO(vish): better way of adding floating ips
>
> === modified file 'nova/utils.py'
> --- nova/utils.py       2010-12-26 14:08:38 +0000
> +++ nova/utils.py       2011-01-05 11:56:16 +0000
> @@ -31,6 +31,8 @@
>  import sys
>  import time
>  from xml.sax import saxutils
> +import re
> +import netaddr
>
>  from eventlet import event
>  from eventlet import greenthread
> @@ -45,6 +47,7 @@
>  def import_class(import_str):
>     """Returns a class from a string including module and class"""
>     mod_str, _sep, class_str = import_str.rpartition('.')
> +    logging.debug(import_str)
>     try:
>         __import__(mod_str)
>         return getattr(sys.modules[mod_str], class_str)

I don't know if this is something you added for temporary debugging, but
if it's going to stay, please don't just log the import_str, but rather
something like (_("Importing %s"), import_str)

> @@ -207,6 +210,37 @@
>         return "127.0.0.1"
>
>
> +def  get_my_linklocal(interface):
> +    try:
> +        if_str = execute("ifconfig %s" % interface)
> +        condition = "\s+inet6\s+addr:\s+([0-9a-f:]+/\d+)\s+Scope:Link"
> +        links = [re.search(condition, x) for x in if_str[0].split('\n')]
> +        address = [w.group(1) for w in links if w is not None]
> +        if address[0] is not None:
> +            return address[0]
> +        else:
> +            return None
> +    except RuntimeError as ex:
> +        logging.warn("Couldn't get Link Local IP of %s :%s", interface, ex)
> +        return 'fe00::'

(Pardon my ignorance) Is this really intended to find "Link local"
addresses (169.254.X.X in ipv4 and fe80::/10 in ipv6)? If not, can you
perhaps find a better name for the method?

Also, I recommend using the ip(8) utility for this sort of thing. It's
much easier to parse. Something like "ip -f inet6 -o addr show
<interface>" should be easy to work with.

> === modified file 'nova/virt/libvirt_conn.py'
> --- nova/virt/libvirt_conn.py   2010-12-30 21:23:14 +0000
> +++ nova/virt/libvirt_conn.py   2011-01-05 11:56:16 +0000
> @@ -860,12 +902,20 @@
>         security_group = db.security_group_get(context.get_admin_context(),
>                                                security_group_id)
>         rule_xml = ""
> +        version = 4

This looks superfluous to me? It gets reset in the loop right before its
used anyway.


> +        v6protocol = {'tcp': 'tcp-ipv6', 'udp': 'udp-ipv6', 'icmp': 'icmpv6'}
>         for rule in security_group.rules:
>             rule_xml += "<rule action='accept' direction='in' priority='300'>"
>             if rule.cidr:
> -                net, mask = _get_net_and_mask(rule.cidr)
> -                rule_xml += "<%s srcipaddr='%s' srcipmask='%s' " % \
> -                            (rule.protocol, net, mask)
> +                version = _get_ip_version(rule.cidr)
> +                if(FLAGS.use_ipv6 and version == 6):
> +                    net, prefixlen = _get_net_and_prefixlen(rule.cidr)
> +                    rule_xml += "<%s srcipaddr='%s' srcipmask='%s' " % \
> +                                (v6protocol[rule.protocol], net, prefixlen)
> +                else:
> +                    net, mask = _get_net_and_mask(rule.cidr)
> +                    rule_xml += "<%s srcipaddr='%s' srcipmask='%s' " % \
> +                                (rule.protocol, net, mask)
>                 if rule.protocol in ['tcp', 'udp']:
>                     rule_xml += "dstportstart='%s' dstportend='%s' " % \
>                                 (rule.from_port, rule.to_port)



-- 
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/