Merge lp://qastaging/~jason-koelker/nova/lp819477 into lp://qastaging/~hudson-openstack/nova/trunk

Couldn't find anything wrong other than a couple doc typos:

1034 + # If no ipv6 subnets are define this will be None

typo -> "defined"

1573 + # None if network with already exists

You accidentally a word

Seems reasonable otherwise. Good piece of functionality

> Couldn't find anything wrong other than a couple doc typos:
>
> 1034 + # If no ipv6 subnets are define this will be
> None
>
> typo -> "defined"
>
> 1573 + # None if network with already exists
>
> You accidentally a word
>
> Seems reasonable otherwise. Good piece of functionality

Typos have fixed. ;)

Hmm, any reason why we'd need that one in Diablo ? We need some stability, and this changes DB and adds a nova-manage command...

ttx: we need this among other reasons to be able to add multiple subnets (with separate gateways) to a network. I've got a hack in place allowing this particular functionality that we use in our branch, but it's getting unwieldy keeping it up to date with trunk.

setting to needs information so this stays on my radar

Trey: I have no idea how common that use case is. I'm just naturally reluctant, after the last milestone, to add any "feature" that modifies the DB... I'd also like to make the pre-release documenters job a little less miserable by not adding new commands to nova-manage in the last month :)

Feel free to convince Vish of the necessity of having that fix in.

The ability to allow for overlaid networks is pretty important especially for a service provider. The current behavior only allows for the addition of a single network block per network for Flat networks. Sometimes we'll need to add several blocks of IPs to a Flat network and with the way the code works currently, it would generate a new VIF per block added. This makes it really hard to scale a zone out. A single flat network can have many overlaid network blocks on it and this patch would allow for that to work correctly.

It also gives us the ability to add smaller blocks at a time and move them around if we need to. The case where this makes sense is if a lot of space gets freed up in a zone while another zone starts nearing IP space capacity.

This definitely seems like good functionality, but I'm also a bit concerned about merging it this late. Trying to get a bit of a risk assesment

Thanks for the fixes, Banadushi

220 +def get_session():
221 + if hasattr(IMPL, 'get_session'):
222 + return IMPL.get_session()
223 +
224 +
225 +###################
226 +
227 +

Don't like exposing the session outside of the db. We need to be able to do this without using the session in nova-manage

Rock on, I remove the leaky session ;).

Happy Hacking!

7-11

While this is a somewhat larger than normal last minute change, I think this looks pretty safe. I'd like to see this make it in.

+1

also, vish: thanks for working with us on this.

Still quite a bit broken. I got most of the way there like this:

http://paste.openstack.org/show/2350/

most of those errors are caught by pyflakes, btw. I highly recommend you get it running automatically in your editor.

Remaining issues:

There are still a bunch of references to instance['fixed_ips'] and fixed['network'] in cloud.py that will cause metadata and describe instances to fail.

The network creation is still failing in my tests because vlan and bridge aren't getting set automatically by the command:
nova-manage network create private 10.0.0.0/24 1 256

vpn_public_address and vpn_public_port really seem like they should be properties of the network, not the subnet, and i worry that anything involving them is seriously broken. Nova-manage commands, ip forwarding rules, cloudpipe launching etc.

Really nice work here Jason.

A couple of micro-nits (would be nice-to-have fixes, but not enough to hold up the patch):

> 847 + i = subnets.insert()

Would prefer non-single-letter-vars, maybe `insert_stmt`.

> 1826 + if not network:
> 1827 + raise ValueError(_('Network already exists!'))
> 1828 + else:
> 1829 + nw_data.append(network)

A bit of a double negative with the else, perhaps:

if network:
    nw_data.append(network)
else:
    raise ValueError(_('Network already exists!'))

lgtm, thanks for the fixups.

> Still quite a bit broken. I got most of the way there like this:
>
> http://paste.openstack.org/show/2350/
>
> most of those errors are caught by pyflakes, btw. I highly recommend you get
> it running automatically in your editor.

Yea installed that, much better.

> Remaining issues:
>
> There are still a bunch of references to instance['fixed_ips'] and
> fixed['network'] in cloud.py that will cause metadata and describe instances
> to fail.

I fixed these. The instance['fixed_ips'] is still valid. The fixed['network'] call I removed and had the vif generate the ip with its property.

> The network creation is still failing in my tests because vlan and bridge
> aren't getting set automatically by the command:
> nova-manage network create private 10.0.0.0/24 1 256

I'm still looking at this.

> vpn_public_address and vpn_public_port really seem like they should be
> properties of the network, not the subnet, and i worry that anything involving
> them is seriously broken. Nova-manage commands, ip forwarding rules,
> cloudpipe launching etc.

Well I *think* that by moving them along with the subnet that you should get one per subnet. But I don't have a setup to test them. I'll see if I can get my xenserver to play with vlan manager nicely.

> > The network creation is still failing in my tests because vlan and bridge
> > aren't getting set automatically by the command:
> > nova-manage network create private 10.0.0.0/24 1 256
>
> I'm still looking at this.

I think this is now taken care of. Please advise if it is not.

> > vpn_public_address and vpn_public_port really seem like they should be
> > properties of the network, not the subnet, and i worry that anything
> involving
> > them is seriously broken. Nova-manage commands, ip forwarding rules,
> > cloudpipe launching etc.
>
> Well I *think* that by moving them along with the subnet that you should get
> one per subnet. But I don't have a setup to test them. I'll see if I can get
> my xenserver to play with vlan manager nicely.

I updated the `nova-manage vpn change` command to update the subnet. I couldn't find any other references to vpn_ in it please let me know if there are others I am missing.

The forwarding rules (I assume ensure_vpn_forward in linux_net) are getting the port and private address passed to them from the subnet table as they are getting called once per subnet per network. This should allow for the updating of the VPN system to handle multiple networks/subnet nicely (currently it just takes the 1st network it finds for a project).

I also updated the auth/manager's get_project_vpn_data to return the data for the 1st v4 subnet on the network. I *think* this is correct and will work, but I don't have the setup to test cloudpipe instances extensively. Please let me know if its still not working as expected.

Essex is open !

Hi Jason,

I'm getting errors when using FlatDHCP w/ this branch (via Smokestack):

  InvalidRequestError: Object '<Subnet at 0x3840510>' is already attached to session

Paste stack trace available here:

  http://paste.openstack.org/show/2413/

---

Also. You have some conflicts to fix with the latest nova trunk:

Text conflict in bin/nova-manage
Text conflict in nova/db/api.py
Text conflict in nova/db/sqlalchemy/api.py
Text conflict in nova/db/sqlalchemy/models.py
Text conflict in nova/exception.py
Text conflict in nova/tests/test_network.py

On Mon, 2011-09-12 at 01:52 +0000, Dan Prince wrote:
> Review: Needs Fixing
>
> Hi Jason,
>
> I'm getting errors when using FlatDHCP w/ this branch (via Smokestack):
>
> InvalidRequestError: Object '<Subnet at 0x3840510>' is already attached to session
>
> Paste stack trace available here:
>
> http://paste.openstack.org/show/2413/

Do you happen to have the full stack trace so I can see where this is
happening at?

> ---
>
> Also. You have some conflicts to fix with the latest nova trunk:
>
> Text conflict in bin/nova-manage
> Text conflict in nova/db/api.py
> Text conflict in nova/db/sqlalchemy/api.py
> Text conflict in nova/db/sqlalchemy/models.py
> Text conflict in nova/exception.py
> Text conflict in nova/tests/test_network.py

Yea, I have the conflicts fixed locally, but the quantum unit tests now
fail since they mock out their own network data. Working on fixing those
before I push again.

Happy Hacking!

7-11

> > Paste stack trace available here:
> >
> > http://paste.openstack.org/show/2413/
>
> Do you happen to have the full stack trace so I can see where this is
> happening at?
>

This one is a bit longer:

http://paste.openstack.org/show/2443/

Sorry. I only have the last 100 lines or so of the nova-network.log file.