Merge lp:~jtran/nova/lp740810 into lp:~hudson-openstack/nova/trunk

[John Tran]

For nova-manage network create cmd, added warning when size of subnet(s) being created are larger than FLAG.network_size, in attempt to alleviate confusion. For example, currently when 'nova-manage network create foo 192.168.0.0/16', the result is that it creates a 192.168.0.0/24 instead without any indication to why

For nova-manage network create cmd, added warning when size of subnet(s) being created are larger than FLAG.network_size, in attempt to alleviate confusion. For example, currently when 'nova-manage network create foo 192.168.0.0/16', the result is that it creates a 192.168.0.0/24 instead without any indication to why

[Brian Waldon (bcwaldon)]

Good catch, John. One thing:

22: you need to put '% subnet' outside of the i18n parentheses

review: Needs Fixing

[John Tran (jtran)]

> Good catch, John. One thing:
>
> 22: you need to put '% subnet' outside of the i18n parentheses

Brian, thanks for catching that. I've pushed the revision but just curious - why is it preferred to be outside of the parentheses?

[Brian Waldon (bcwaldon)]

> > Good catch, John. One thing:
> >
> > 22: you need to put '% subnet' outside of the i18n parentheses
>
> Brian, thanks for catching that. I've pushed the revision but just curious -
> why is it preferred to be outside of the parentheses?

The best way to explain is through an example. The msgid below is what is found in nova within the i18n parens, and the msgstr is an example japanese translation:

msgid "Quota exceeeded for %(pid)s, tried to create %(size)sG volume"
msgstr "サイズ %(size)s GB のボリューム作成を行おうとしましたが、%(pid)s のサイ>ズ制限を超えています。"

If the substitutions were placed inside of the i18n parens, we would have to match every possible combination of pid and volume:

msgid "Quota exceeeded for 1234, tried to create 10G volume"
msgstr "サイズ 10 GB のボリューム作成を行おうとしましたが、1234 のサイ>ズ制限を超えています。"

msgid "Quota exceeeded for 5678, tried to create 20G volume"
msgstr "サイズ 20 GB のボリューム作成を行おうとしましたが、5678 のサイ>ズ制限を超えています。"

msgid "Quota exceeeded for 1029, tried to create 30G volume"
msgstr "サイズ 30 GB のボリューム作成を行おうとしましたが、1029 のサイ>ズ制限を超えています。"

In addition to placing the substitution arguments outside of the i18n parens, we also want to use keyword params when we have more than one value to place within the string. That allows the translator to change the order of the params as needed. I hope this makes sense!

review: Approve

[Trey Morris (tr3buchet)]

John, this bug does need fixing but this is not the place for it. We don't want to be doing anything in nova-manage besides collecting the arguments together to pass off to the code that does the real work. In this case the real work is done in nova/network/manager.py by a couple of functions both called "create_networks".

In this case it looks as though there is a paragraph of code in the vlanmanager version of that function that should be moved into the superclass version:

875 # check that num networks and network size fits in fixed_net
876 fixed_net = netaddr.IPNetwork(kwargs['cidr'])
877 if len(fixed_net) < kwargs['num_networks'] * kwargs['network_size']:
878 raise ValueError(_('The network range is not big enough to fit '
879 '%(num_networks)s. Network size is %(network_size)s') %
880 kwargs)

review: Disapprove

[John Tran (jtran)]

> John, this bug does need fixing but this is not the place for it. We don't
> want to be doing anything in nova-manage besides collecting the arguments
> together to pass off to the code that does the real work. In this case the
> real work is done in nova/network/manager.py by a couple of functions both
> called "create_networks".
>
> In this case it looks as though there is a paragraph of code in the
> vlanmanager version of that function that should be moved into the superclass
> version:
>
>
> 875 # check that num networks and network size fits in fixed_net
> 876 fixed_net = netaddr.IPNetwork(kwargs['cidr'])
> 877 if len(fixed_net) < kwargs['num_networks'] *
> kwargs['network_size']:
> 878 raise ValueError(_('The network range is not big enough to fit
> '
> 879 '%(num_networks)s. Network size is %(network_size)s') %
> 880 kwargs)

Try, the intention for the bug fix doesn't address the same issue you've highlighted. The server side bug you've highlighted is meant to address subnet size calculation issues. The bug I'm fixing is when using the nova-manage script, if you specify to create a network '192.168.0.0/16' , it'll automatically default back to '192.168.0.0/24' (or whatever is specified in the flags file. That causes confusion. The logic that does the reverting to default flag exists only in the nova-manage client and is there for developer preference as far as I understand it. The bug fix I added simply gives some verbosity so that end users using the nova-manage client understand why their cidr specified got converted.

[Trey Morris (tr3buchet)]

I see. This is a flags problem you fixed. I think this is all a bit silly, I don't like when arguments to a command come from multiple sources, especially when some are hidden within the code. This could easily be solved by removing the flags for num_networks and network_size and requiring the user pass in these values via commandline to nova-manage network create. This way there would be no surprises.

Alternatively, we could remove the default values for the 2 flags. In this way the user would at least be responsible for setting values that make sense in their implementation, and again no surprises.

Due to the fact that networks will probably be added in many different sizes through the course of time, I'd like to suggest we remove these two flags. They are very minimally referred to in code, each is used only once in nova-manage network create, and if I'm right about adding networks of different sizes, the values for these flags will rarely be used as well. I've tweaked this command before, I'll gladly update it to work without those two flags if this is acceptable.

Otherwise, I have no problems with your fix. I think it will work great if keeping the flags is the way we end up going. Please merge trunk as this particular command is greatly changed from how it appears in your branch.

[Vish Ishaya (vishvananda)]

setting to WIP until trunk is merged.

review: Approve

[John Tran (jtran)]

> setting to WIP until trunk is merged.

I've merged from trunk. Thanks!

[lp:~jtran/nova/lp740810]

1317. By John Tran on 2011-08-03

forgot to remove comment

[Soren Hansen (soren)]

I would love to see some tests for this.

[Trey Morris (tr3buchet)]

soren, i'd like to see some too, but i don't think there are any tests for the nova-manage commands... Maybe a blueprint is the proper place for this?

review: Approve

=== modified file 'bin/nova-manage'
--- bin/nova-manage	2011-07-29 21:10:45 +0000
+++ bin/nova-manage	2011-08-03 07:17:47 +0000
@@ -56,6 +56,7 @@
 import gettext
 import glob
 import json
+import math
 import netaddr
 import os
 import sys
@@ -694,7 +695,17 @@
         if not num_networks:
             num_networks = FLAGS.num_networks
         if not network_size:
-            network_size = FLAGS.network_size
+            fixnet = netaddr.IPNetwork(fixed_range_v4)
+            each_subnet_size = fixnet.size / int(num_networks)
+            if each_subnet_size > FLAGS.network_size:
+                network_size = FLAGS.network_size
+                subnet = 32 - int(math.log(network_size, 2))
+                oversize_msg = _('Subnet(s) too large, defaulting to /%s.'
+                                 '  To override, specify network_size flag.'
+                                ) % subnet
+                print oversize_msg
+            else:
+                network_size = fixnet.size
         if not multi_host:
             multi_host = FLAGS.multi_host
         else: