Merge lp://qastaging/~chemikadze/nova/contrib-extention-networks into lp://qastaging/~hudson-openstack/nova/trunk

 I got errors when I run unittest.
Plz check it
http://paste.openstack.org/show/2228/

I didn't expect that adding an extention may cause other test failures, I'll be more careful in future. Fixed that problems.

I haven't done a full review yet, but I was curious how/if this is currently work for you. It seems like you're setting up self.network_api, but never using it?

69 + self.network_api = network.API()

Does this not need to be used anywhere?

Thanks again, Brian. Yes, you are right, I mistakely got this code from FloatingIPs extention, what I have used as pattern during porting my code from openstackx to nova. Now I understand extention mechanics in OS a little bit more, so that can be deleted.

This should wait for Essex.

I'm not really a fan of this being an admin route, exactly. Currently there are no other extensions that use the "admin" route or any standard spec controllers that do. I would actually suggest we simply remove "admin" from the route, because I think it could be confusing to the end user trying to discover functional
ity about the API. Specifically, they might wonder why other "admin" looking actions are routed under existing controllers, and what about "networks" makes it specifically admin related.

7 +# Copyright 2011 OpenStack LLC.
8 +# Copyright 2011 Grid Dynamics
9 +# Copyright 2011 Nikolay Sokolov

If you're adding a new file, you only need whichever copyright is most relevant to the work you're doing. I'm guessing this would primarily be Grid Dynamics in this case?

I'm concerned about the unit tests in this patch, too. While I endorse the idea that we don't use the database for unit tests, I'm afraid that, given your overriding every single database API method, if the networks tables were to change in any way, your tests would continue to pass. What do you think?

Matt, thanks for your comments.

I checked admin api bluprints and now see that unlike openstackx, core nova extentions doesn't use special admin route. So I changed it to common-style 'os-networks'.

Copyrights were made like in floating ips extention, and I thought that if it is in trunk, then it is ok. I'm master in legal issues, but reviewing other sources that my patch is outstanding in that case too, and I fixed that.

But the last one is not clear to me: I agree with thought, that tests passing on mock underlaying code and broken with real code is not good (by the way, just today i've read Sandy's article about that). I tried to find sample of how to deal with that, but I didn't find it neither in extention tests, nor in core OS API tests. In that particular case, I think there is possible to write separate test case, just checking that "fake net" object fits to database. But in general, I'm confused that other tests doesn't fit that requirement. From the other side, isn't verifying backward compatability a problem of database tests?

Essex is open !

Sorry for the delay in review,

66 + def _network_get_autodetect(self, context, id):

I don't love the need for this function, but I suppose it's a design decision to allow for multiple types of network identifiers to be passed to show/delete/index/etc. It's probably not something I'd hold the merge prop up on, but just stating that I'd rather only have 1 type of input instead of 2 or 3.

122 + except exception.NetworkNotFound:
123 + query_params = urlparse.parse_qs(req.environ['QUERY_STRING'])
124 + cidrs = query_params.get('cidr')
125 + if cidrs:
126 + for cidr in cidrs:
127 + if id == 'disassociate':
128 + self.disassociate(req, cidr)
129 + elif id == 'delete':
130 + self.delete(req, cidr)
131 + return exc.HTTPAccepted()

At first this exception handling was a bit confusing to me, then I realized it was because this method is handling multiple cases:

DELETE /v1.1/os-networks/networks/{id}
DELETE /v1.1/os-networks/networks/delete?cidr={cidr}
DELETE /v1.1/os-networks/networks/disassociate?cidr={cidr}

You're using special identifiers 'delete' and 'disassociate' to allow for the deletion or disassociation of multiple networks at the same time. IMO this is something we should avoid and I'd rather have one way to do things instead of 2. Also there aren't any prior API calls that I'm aware of that do this (multiple deletions at the same time) so it would be more 'standard' to remove this feature and only allow deletions and disassociations by integer identifier. It's quite confusing to call DELETE on /v1.1/os-networks/networks/disassociate, it's double negative-ish.

447 + "NetworkAdmin",

Can you replace these tabs with spaces?

When I wrote that extention, I was confused about API too: from one side, it is nice to have possibility to manipulate networks by CIDR, like using nova-manage, from another -- resulting API was conceptually unclear. "Feature creature" won at that time, but now
after serveral weeks passed I totally agree with you (especially after viewing OS API -- it seemed fairy nice and simple to me). What do you think about changing resulting IP to something like that:

Delete networks:
DELETE /os-networks/{id}

Disassociate networks:
DELETE /os-networks/{id}/association

List all networks:
GET /os-networks/

Get specific network:
GET /os-networks/{id}

Not sure if you're in IRC, but I'm curious what:

DELETE /os-networks/{id}/association

does still. It looks like it just calls DB's network_disassociate() method which updates the project_id and host of the network to None. If we're following other OpenStack APIs I think a:

POST /os-networks/{id}/actions

with a body of:

{
    "disassociate": null
}

might be the most compliant. It seems like this lines up with our server "actions" (http://docs.openstack.org/cactus/openstack-compute/developer/openstack-compute-api-1.1/content/Server_Actions-d1e3229.html)

That being said this is a pretty big switch from what you have so I'm open for talking more about the best approach! Other than that I think:

Delete networks:
DELETE /os-networks/{id}

List all networks:
GET /os-networks/

Get specific network:
GET /os-networks/{id}

Looks great.

It is not hard to me make this extention more close to nova api. You can check it out just now.

Thanks for the fixes, but noticed something else

In the test methods where you use the global keyword, you actually don't need it. Python is a little opaque on how it handles scoping, but in the methods where you don't actually reassign fake_nets, you don't need global.

See http://paste.openstack.org/show/2487/

300 + global fake_nets
301 + fake_nets = init_fake_nets()

You *would* need it here, because you're assigning it.

Yes, I know. Wrote those extra lines just to emphasize with what these functions actually work. If you don't like such redurancy, I can fix but don't see real reason for that.

I'd argue the code speaks for itself, and doesn't need global constructs
to highlight it. I'd prefer you fix it for brevity's sake.

On 9/22/11 1:21 AM, "Nikolay Sokolov" <email address hidden> wrote:

>Yes, I know. Wrote those extra lines just to emphasize with what these
>functions actually work. If you don't like such redurancy, I can fix but
>don't see real reason for that.
>--
>https://code.launchpad.net/~chemikadze/nova/contrib-extention-networks/+me
>rge/72204
>You are reviewing the proposed merge of
>lp:~chemikadze/nova/contrib-extention-networks into lp:nova.

This email may include confidential information. If you received it in error, please delete it.