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

[Nikolay Sokolov]

This contrib extention adds some admin-level API into Openstack Nova.

In that extention implemented non-single-use methods of nova-manage-network: disassociating, deleting, listing.

Delete networks:
DELETE /admin/networks/{id}
DELETE /admin/networks/delete?cidr={cidr}

Disassociate networks:
DELETE /admin/networks/{id}/disassociate
DELETE /admin/networks/disassociate?cidr={cidr}

List all networks:
GET /admin/networks/

Get specifiq network:
GET /admin/networks/{id}
GET /admin/networks?cidr={cidr}

[Nachi Ueno (nati-ueno)]

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

[lp:~chemikadze/nova/contrib-extention-networks]

1456. By Nikolay Sokolov on 2011-08-20

Fixed log formatting and extention tests.

[Nikolay Sokolov (chemikadze)]

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

[Brian Lamar (blamar)]

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?

review: Needs Information

[lp:~chemikadze/nova/contrib-extention-networks]

1457. By Nikolay Sokolov on 2011-08-23

Removed dead code

[Nikolay Sokolov (chemikadze)]

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.

[lp:~chemikadze/nova/contrib-extention-networks]

1458. By Nikolay Sokolov on 2011-08-23

Consistent get_updated

[Thierry Carrez (ttx)]

This should wait for Essex.

review: Disapprove

[Thierry Carrez (ttx)]

review: Needs Information (ffe)

[Matt Dietz (cerberus)]

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?

review: Needs Fixing

[lp:~chemikadze/nova/contrib-extention-networks]

1459. By Nikolay Sokolov on 2011-09-06

Changed endpoint to os-networks

1460. By Nikolay Sokolov on 2011-09-06

Updated copyright

[Nikolay Sokolov (chemikadze)]

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?

[Thierry Carrez (ttx)]

Essex is open !

review: Abstain (ffe)

[Brian Lamar (blamar)]

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?

review: Needs Fixing

[Nikolay Sokolov (chemikadze)]

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}

[lp:~chemikadze/nova/contrib-extention-networks]

1461. By Nikolay Sokolov on 2011-09-19

Fixed issue with tabs

[Brian Lamar (blamar)]

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.

[lp:~chemikadze/nova/contrib-extention-networks]

1462. By Nikolay Sokolov on 2011-09-20

More openstack-style IP

1463. By Nikolay Sokolov on 2011-09-20

pep8 fix

[Nikolay Sokolov (chemikadze)]

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

[Matt Dietz (cerberus)]

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.

[Nikolay Sokolov (chemikadze)]

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.

[Matt Dietz (cerberus)]

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.

Unmerged revisions

1463. By Nikolay Sokolov on 2011-09-20

pep8 fix

1462. By Nikolay Sokolov on 2011-09-20

More openstack-style IP

1461. By Nikolay Sokolov on 2011-09-19

Fixed issue with tabs

1460. By Nikolay Sokolov on 2011-09-06

Updated copyright

1459. By Nikolay Sokolov on 2011-09-06

Changed endpoint to os-networks

1458. By Nikolay Sokolov on 2011-08-23

Consistent get_updated

1457. By Nikolay Sokolov on 2011-08-23

Removed dead code

1456. By Nikolay Sokolov on 2011-08-20

Fixed log formatting and extention tests.

1455. By Nikolay Sokolov on 2011-08-19

pep8

1454. By Nikolay Sokolov on 2011-08-19

Deleting by CIDR

=== added file 'nova/api/openstack/contrib/networks.py'
--- nova/api/openstack/contrib/networks.py	1970-01-01 00:00:00 +0000
+++ nova/api/openstack/contrib/networks.py	2011-09-20 09:08:24 +0000
@@ -0,0 +1,148 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Grid Dynamics
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import urlparse
+
+from webob import exc
+
+from nova import db
+from nova import exception
+from nova import flags
+from nova import log as logging
+
+from nova.api.openstack import extensions
+from nova.api.openstack.contrib.admin_only import admin_only
+
+FLAGS = flags.FLAGS
+
+LOG = logging.getLogger('nova.api.contrib.networks')
+
+
+def network_dict(network):
+    if network:
+        fields = (
+        'bridge', 'vpn_public_port', 'dhcp_start', 'bridge_interface',
+        'updated_at', 'id', 'cidr_v6', 'deleted_at',
+        'gateway', 'label', 'project_id', 'vpn_private_address',
+        'deleted',
+        'vlan', 'broadcast', 'netmask', 'injected',
+        'cidr', 'vpn_public_address', 'multi_host', 'dns1', 'host',
+        'gateway_v6', 'netmask_v6', 'created_at')
+        return dict((field, getattr(network, field)) for field in fields)
+    else:
+        return {}
+
+
+def require_admin(f):
+    def wraps(self, req, *args, **kwargs):
+        if 'nova.context' in req.environ and\
+           req.environ['nova.context'].is_admin:
+            return f(self, req, *args, **kwargs)
+        else:
+            raise exception.AdminRequired()
+    return wraps
+
+
+class NetworkController(object):
+
+    @require_admin
+    def action(self, req, id, body):
+        actions = {'disassociate': self._disassociate}
+        for action, data in body.iteritems():
+            try:
+                return actions[action](req, id, body)
+            except KeyError:
+                msg = _("Network does not have %s action") % action
+                raise exc.HTTPBadRequest(explanation=msg)
+        msg = _("Invalid request body")
+        raise exc.HTTPBadRequest(explanation=msg)
+
+    def _disassociate(self, req, id, body):
+        context = req.environ['nova.context']
+        LOG.debug(_("Disassociating network with id %{query}s, "
+                    "context %{context}s"),
+                {"query": id, "context": context})
+        try:
+            net = db.network_get(context, int(id))
+        except exception.NetworkNotFound:
+            raise exc.HTTPNotFound()
+        db.network_disassociate(context, net.id)
+        return {'disassociated': int(id)}
+
+    @require_admin
+    def index(self, req):
+        """Can filter projects """
+        context = req.environ['nova.context']
+        LOG.info(_("Getting networks with context %{ctxt}s"),
+                 {"ctxt": context})
+        networks = db.network_get_all(context)
+        result = [network_dict(net_ref) for net_ref in networks]
+        return  {'networks': result}
+
+    @require_admin
+    def show(self, req, id):
+        context = req.environ['nova.context']
+        LOG.info(_("Showing network with id %{query}s, context %{ctxt}s"),
+                 {"query": id, "context": context})
+        try:
+            net = db.network_get(context, int(id))
+        except exception.NetworkNotFound:
+            raise exc.HTTPNotFound()
+        return {'network': network_dict(net)}
+
+    @require_admin
+    def delete(self, req, id):
+        context = req.environ['nova.context']
+        LOG.audit(_("Deleting network with id %{query}s, context %{ctxt}s"),
+                  {"query": context, "ctxt": context})
+        try:
+            net = db.network_get(context, int(id))
+        except exception.NetworkNotFound:
+            raise exc.HTTPNotFound()
+        db.network_delete_safe(context, net.id)
+        return exc.HTTPAccepted()
+
+        # TODO(nsokolov): implement full CRUD, not done right in nova too
+
+
+class Networks(extensions.ExtensionDescriptor):
+    def __init__(self):
+        pass
+
+    def get_name(self):
+        return "NetworkAdmin"
+
+    def get_alias(self):
+        return "NETWORK"
+
+    def get_description(self):
+        return "The Network API Extension"
+
+    def get_namespace(self):
+        return "http://docs.openstack.org/ext/os-networks/api/v1.1"
+
+    def get_updated(self):
+        return "2011-08-23 07:44:50.888131"
+
+    @admin_only
+    def get_resources(self):
+        resources = []
+        resources.append(extensions.ResourceExtension('os-networks',
+                                                      NetworkController(),
+                                                      member_actions={
+                                                          'action': 'POST'}))
+        return resources

=== added file 'nova/tests/api/openstack/contrib/test_networks.py'
--- nova/tests/api/openstack/contrib/test_networks.py	1970-01-01 00:00:00 +0000
+++ nova/tests/api/openstack/contrib/test_networks.py	2011-09-20 09:08:24 +0000
@@ -0,0 +1,229 @@
+# Copyright 2011 Grid Dynamics
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import json
+from urllib import urlencode
+import webob
+
+from nova import context
+from nova import db
+from nova.db.sqlalchemy.api import require_admin_context, require_context
+from nova import exception
+from nova import test
+from nova.tests.api.openstack import fakes
+
+from nova.flags import FLAGS
+
+from nova.api.openstack.contrib.networks import NetworkController
+
+
+class fake_ref(dict):
+    def __getattr__(self, item):
+        return self[item]
+
+
+def init_fake_nets():
+    return [fake_ref({'bridge': 'br100', 'vpn_public_port': 1000,
+                      'dhcp_start': '10.0.0.3', 'bridge_interface': 'eth0',
+                      'updated_at': '2011-08-16 09:26:13.048257', 'id': 1,
+                      'cidr_v6': None, 'deleted_at': None,
+                      'gateway': '10.0.0.1', 'label': 'mynet_0',
+                      'project_id': 'admin',
+                      'vpn_private_address': '10.0.0.2', 'deleted': False,
+                      'vlan': 100, 'broadcast': '10.0.0.7',
+                      'netmask': '255.255.255.248', 'injected': False,
+                      'cidr': '10.0.0.0/29',
+                      'vpn_public_address': '127.0.0.1', 'multi_host': False,
+                      'dns1': None, 'host': 'nsokolov-desktop',
+                      'gateway_v6': None, 'netmask_v6': None,
+                      'created_at': '2011-08-15 06:19:19.387525'}),
+            fake_ref({'bridge': 'br101', 'vpn_public_port': 1001,
+                      'dhcp_start': '10.0.0.11', 'bridge_interface': 'eth0',
+                      'updated_at': None, 'id': 2, 'cidr_v6': None,
+                      'deleted_at': None, 'gateway': '10.0.0.9',
+                      'label': 'mynet_1', 'project_id': None,
+                      'vpn_private_address': '10.0.0.10', 'deleted': False,
+                      'vlan': 101, 'broadcast': '10.0.0.15',
+                      'netmask': '255.255.255.248', 'injected': False,
+                      'cidr': '10.0.0.10/29', 'vpn_public_address': None,
+                      'multi_host': False, 'dns1': None, 'host': None,
+                      'gateway_v6': None, 'netmask_v6': None,
+                      'created_at': '2011-08-15 06:19:19.885495'})]
+
+fake_nets = init_fake_nets()
+
+
+@require_admin_context
+def db_network_disassociate(context, id):
+    global fake_nets
+    for net in fake_nets:
+        if net['id'] == id:
+            net['project_id'] = None
+            return net
+    raise exception.NetworkNotFound()
+
+
+@require_admin_context
+def db_network_get_all(context):
+    global fake_nets
+    return fake_nets
+
+
+@require_context
+def db_project_get_networks(context, tenant_id, associate=False):
+    global fake_nets
+    tenant_nets = []
+    for net in fake_nets:
+        if net['project_id'] == tenant_id:
+            tenant_nets.append(net)
+    return tenant_nets
+
+
+@require_context
+def db_network_get(context, network_id):
+    global fake_nets
+    for net in fake_nets:
+        if net['id'] == network_id:
+            return net
+    raise exception.NetworkNotFound()
+
+
+@require_context
+def db_network_get_by_cidr(context, cidr):
+    global fake_nets
+    for net in fake_nets:
+        if net['cidr'] == cidr:
+            return net
+    raise exception.NetworkNotFound()
+
+
+@require_admin_context
+def db_network_delete(context, network_id):
+    global fake_nets
+    fake_nets.remove(db.network_get(context, network_id))
+
+
+class NetworkExtentionTest(test.TestCase):
+    def setUp(self):
+        super(NetworkExtentionTest, self).setUp()
+        FLAGS.allow_admin_api = True
+        self.controller = NetworkController()
+        fakes.stub_out_networking(self.stubs)
+        fakes.stub_out_rate_limiting(self.stubs)
+        self.stubs.Set(db, "network_disassociate",
+                       db_network_disassociate)
+        self.stubs.Set(db, "network_get_all",
+                       db_network_get_all)
+        self.stubs.Set(db, "project_get_networks",
+                       db_project_get_networks)
+        self.stubs.Set(db, "network_get",
+                       db_network_get)
+        self.stubs.Set(db, "network_get_by_cidr",
+                       db_network_get_by_cidr)
+        self.stubs.Set(db, "network_delete_safe",
+                       db_network_delete)
+        self.user = 'user'
+        self.project = 'project'
+        self.user_context = context.RequestContext(self.user, self.project,
+                                                   is_admin=False)
+        self.admin_context = context.RequestContext(self.user, self.project,
+                                                    is_admin=True)
+        global fake_nets
+        fake_nets = init_fake_nets()
+
+    def test_network_list_all(self):
+        req = webob.Request.blank('/v1.1/os-networks')
+        req.method = 'GET'
+        req.headers['Content-Type'] = 'application/json'
+
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 200)
+        res_dict = json.loads(res.body)
+        self.assertEquals(res_dict, {'networks': fake_nets})
+
+        req = webob.Request.blank('/v1.1/os-networks')
+        req.method = 'GET'
+        req.headers['Content-Type'] = 'application/json'
+
+        with self.assertRaises(exception.AdminRequired):
+            res = req.get_response(fakes.wsgi_app(
+                fake_auth_context=self.user_context))
+            self.assertEqual(res.status_int, 500)  # admin required
+            res_dict = json.loads(res.body)
+
+            right_ans = {'networks': db_network_get_all(self.user_context)}
+
+    def test_network_disassociate(self):
+        req = webob.Request.blank('/v1.1/os-networks/1/action')
+        req.method = 'POST'
+        req.body = json.dumps({'disassociate': None})
+        req.headers['Content-Type'] = 'application/json'
+
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 200)
+        self.assertEqual(json.loads(res.body), {'disassociated': 1})
+        self.assertEqual(db.network_get(self.admin_context, 1)['project_id'],
+                         None)
+
+        req = webob.Request.blank(
+            '/v1.1/os-networks/12345/action')  # not present
+        req.method = 'POST'
+        req.body = json.dumps({'disassociate': None})
+        req.headers['Content-Type'] = 'application/json'
+
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 404)
+
+    def test_network_get(self):
+        req = webob.Request.blank('/v1.1/os-networks/1')
+        req.method = 'GET'
+        req.headers['Content-Type'] = 'application/json'
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 200)
+        res_dict = json.loads(res.body)
+        waited = {'network': db_network_get(self.admin_context, 1)}
+        self.assertEquals(res_dict, waited)
+
+        req = webob.Request.blank('/v1.1/os-networks/1')
+        req.headers['Content-Type'] = 'application/json'
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.user_context))
+        self.assertEqual(res.status_int, 500)
+
+    def test_network_delete(self):
+        req = webob.Request.blank('/v1.1/os-networks/1')
+        req.method = 'DELETE'
+        req.headers['Content-Type'] = 'application/json'
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 202)
+        # check it was really deleted
+        req = webob.Request.blank('/v1.1/os-networks/1')
+        req.method = 'GET'
+        req.headers['Content-Type'] = 'application/json'
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 404)
+
+        req = webob.Request.blank('/v1.1/os-networks/12345')  # not present
+        req.method = 'DELETE'
+        req.headers['Content-Type'] = 'application/json'
+        res = req.get_response(fakes.wsgi_app(
+            fake_auth_context=self.admin_context))
+        self.assertEqual(res.status_int, 404)

=== modified file 'nova/tests/api/openstack/test_extensions.py'
--- nova/tests/api/openstack/test_extensions.py	2011-09-14 15:54:56 +0000
+++ nova/tests/api/openstack/test_extensions.py	2011-09-20 09:08:24 +0000
@@ -93,6 +93,7 @@
             "Hosts",
             "Keypairs",
             "Multinic",
+            "NetworkAdmin",
             "Quotas",
             "Rescue",
             "SecurityGroups",