Make flavor-manage api call destroy with Flavor object

The flavor-manage api delete command currently need 3 separate db calls
to remove a flavor from the db. This is due to the use of flavor name to
identify the db flavor when the api command itself take flavorid as
input.

This change reworks this to only issue a single db call that now use
flavorid all way through to the db. It also gets rid of the in between
flavors module by calling destroy on a Flavor object right from the api.

Additional test changes are made to similarily bypass the flavors module
and instead call destroy() directly on the Flavor objects so the flavors
method becomes unused and thus can be removed.

Change-Id: I62bc11887283201be0bcdcfe9fa58b3f6156c754

11 files changed, 47 insertions, 92 deletions

diff --git a/nova/api/openstack/compute/flavor_manage.py b/nova/api/openstack/compute/flavor_manage.py
index 4cd936ab1a..b4b73f415d 100644
--- a/nova/api/openstack/compute/flavor_manage.py
+++ b/nova/api/openstack/compute/flavor_manage.py
@@ -20,6 +20,7 @@ from nova.api import validation
 from nova.compute import flavors
 from nova import exception
 from nova.i18n import _
+from nova import objects
 
 ALIAS = "os-flavor-manage"
 
@@ -43,14 +44,12 @@ class FlavorManageController(wsgi.Controller):
         context = req.environ['nova.context']
         authorize(context)
 
+        flavor = objects.Flavor(context=context, flavorid=id)
         try:
-            flavor = flavors.get_flavor_by_flavor_id(
-                    id, ctxt=context, read_deleted="no")
+            flavor.destroy()
         except exception.FlavorNotFound as e:
             raise webob.exc.HTTPNotFound(explanation=e.format_message())
 
-        flavors.destroy(flavor['name'])
-
     # NOTE(oomichi): Return 200 for backwards compatibility but should be 201
     # as this operation complete the creation of flavor resource.
     @wsgi.action("create")
diff --git a/nova/api/openstack/compute/legacy_v2/contrib/flavormanage.py b/nova/api/openstack/compute/legacy_v2/contrib/flavormanage.py
index 073ec965e8..7d8deaa1ad 100644
--- a/nova/api/openstack/compute/legacy_v2/contrib/flavormanage.py
+++ b/nova/api/openstack/compute/legacy_v2/contrib/flavormanage.py
@@ -18,6 +18,7 @@ from nova.api.openstack import wsgi
 from nova.compute import flavors
 from nova import exception
 from nova.i18n import _
+from nova import objects
 
 
 authorize = extensions.extension_authorizer('compute', 'flavormanage')
@@ -34,14 +35,13 @@ class FlavorManageController(wsgi.Controller):
     def _delete(self, req, id):
         context = req.environ['nova.context']
         authorize(context)
+
+        flavor = objects.Flavor(context=context, flavorid=id)
         try:
-            flavor = flavors.get_flavor_by_flavor_id(
-                    id, ctxt=context, read_deleted="no")
+            flavor.destroy()
         except exception.FlavorNotFound as e:
             raise webob.exc.HTTPNotFound(explanation=e.format_message())
 
-        flavors.destroy(flavor['name'])
-
         return webob.Response(status_int=202)
 
     @wsgi.action("create")
diff --git a/nova/compute/flavors.py b/nova/compute/flavors.py
index ded0e35055..660923da76 100644
--- a/nova/compute/flavors.py
+++ b/nova/compute/flavors.py
@@ -21,7 +21,6 @@
 import re
 import uuid
 
-from oslo_log import log as logging
 from oslo_utils import strutils
 import six
 
@@ -31,14 +30,11 @@ from nova import context
 from nova import db
 from nova import exception
 from nova.i18n import _
-from nova.i18n import _LE
 from nova import objects
 from nova import utils
 
 CONF = nova.conf.CONF
 
-LOG = logging.getLogger(__name__)
-
 # NOTE(luisg): Flavor names can include non-ascii characters so that users can
 # create flavor names in locales that use them, however flavor IDs are limited
 # to ascii characters.
@@ -168,18 +164,6 @@ def create(name, memory, vcpus, root_gb, ephemeral_gb=0, flavorid=None,
     return flavor
 
 
-def destroy(name):
-    """Marks flavor as deleted."""
-    try:
-        if not name:
-            raise ValueError()
-        flavor = objects.Flavor(context=context.get_admin_context(), name=name)
-        flavor.destroy()
-    except (ValueError, exception.NotFound):
-        LOG.exception(_LE('Instance type %s not found for deletion'), name)
-        raise exception.FlavorNotFoundByName(flavor_name=name)
-
-
 def get_all_flavors_sorted_list(ctxt=None, filters=None, sort_key='flavorid',
                                 sort_dir='asc', limit=None, marker=None):
     """Get all non-deleted flavors as a sorted list.
diff --git a/nova/db/api.py b/nova/db/api.py
index a5bc8fe167..0036829872 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -1527,9 +1527,9 @@ def flavor_get_by_flavor_id(context, id, read_deleted=None):
     return IMPL.flavor_get_by_flavor_id(context, id, read_deleted)
 
 
-def flavor_destroy(context, name):
+def flavor_destroy(context, flavor_id):
     """Delete an instance type."""
-    return IMPL.flavor_destroy(context, name)
+    return IMPL.flavor_destroy(context, flavor_id)
 
 
 def flavor_access_get_by_flavor_id(context, flavor_id):
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 416fad8b10..0407e62ff0 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -5204,13 +5204,13 @@ def flavor_get_by_flavor_id(context, flavor_id, read_deleted):
 
 
 @main_context_manager.writer
-def flavor_destroy(context, name):
+def flavor_destroy(context, flavor_id):
     """Marks specific flavor as deleted."""
     ref = model_query(context, models.InstanceTypes, read_deleted="no").\
-                filter_by(name=name).\
+                filter_by(flavorid=flavor_id).\
                 first()
     if not ref:
-        raise exception.FlavorNotFoundByName(flavor_name=name)
+        raise exception.FlavorNotFound(flavor_id=flavor_id)
 
     ref.soft_delete(context.session)
     model_query(context, models.InstanceTypeExtraSpecs, read_deleted="no").\
diff --git a/nova/objects/flavor.py b/nova/objects/flavor.py
index dc8efd8b3e..0007b2099e 100644
--- a/nova/objects/flavor.py
+++ b/nova/objects/flavor.py
@@ -168,20 +168,17 @@ def _flavor_create(context, values):
 
 
 @db_api.api_context_manager.writer
-def _flavor_destroy(context, flavor_id=None, name=None):
+def _flavor_destroy(context, flavor_id=None, flavorid=None):
     query = context.session.query(api_models.Flavors)
 
     if flavor_id is not None:
         query = query.filter(api_models.Flavors.id == flavor_id)
     else:
-        query = query.filter(api_models.Flavors.name == name)
+        query = query.filter(api_models.Flavors.flavorid == flavorid)
     result = query.first()
 
     if not result:
-        if flavor_id is not None:
-            raise exception.FlavorNotFound(flavor_id=flavor_id)
-        else:
-            raise exception.FlavorNotFoundByName(flavor_name=name)
+        raise exception.FlavorNotFound(flavor_id=(flavor_id or flavorid))
 
     context.session.query(api_models.FlavorProjects).\
         filter_by(flavor_id=result.id).delete()
@@ -576,8 +573,8 @@ class Flavor(base.NovaPersistentObject, base.NovaObject,
             self.save_projects(added_projects, deleted_projects)
 
     @staticmethod
-    def _flavor_destroy(context, flavor_id=None, name=None):
-        return _flavor_destroy(context, flavor_id=flavor_id, name=name)
+    def _flavor_destroy(context, flavor_id=None, flavorid=None):
+        return _flavor_destroy(context, flavor_id=flavor_id, flavorid=flavorid)
 
     @base.remotable
     def destroy(self):
@@ -591,9 +588,9 @@ class Flavor(base.NovaPersistentObject, base.NovaObject,
             if 'id' in self:
                 self._flavor_destroy(self._context, flavor_id=self.id)
             else:
-                self._flavor_destroy(self._context, name=self.name)
+                self._flavor_destroy(self._context, flavorid=self.flavorid)
         except exception.FlavorNotFound:
-            db.flavor_destroy(self._context, self.name)
+            db.flavor_destroy(self._context, self.flavorid)
 
 
 @db_api.api_context_manager.reader
@@ -721,7 +718,7 @@ def migrate_flavors(ctxt, count, hard_delete=False):
             if hard_delete:
                 _destroy_flavor_hard(ctxt, flavor.name)
             else:
-                db.flavor_destroy(ctxt, flavor.name)
+                db.flavor_destroy(ctxt, flavor.flavorid)
         except exception.FlavorNotFound:
             LOG.warning(_LW('Flavor id %(id)i disappeared during migration'),
                         {'id': flavor_id})
diff --git a/nova/tests/functional/db/test_flavor.py b/nova/tests/functional/db/test_flavor.py
index 6ff52030c7..5d0574a955 100644
--- a/nova/tests/functional/db/test_flavor.py
+++ b/nova/tests/functional/db/test_flavor.py
@@ -61,7 +61,7 @@ class FlavorObjectTestCase(test.NoDBTestCase):
     def _delete_main_flavors(self):
         flavors = db_api.flavor_get_all(self.context)
         for flavor in flavors:
-            db_api.flavor_destroy(self.context, flavor['name'])
+            db_api.flavor_destroy(self.context, flavor['flavorid'])
 
     def test_create(self):
         self._delete_main_flavors()
@@ -181,13 +181,13 @@ class FlavorObjectTestCase(test.NoDBTestCase):
         flavor = objects.Flavor.get_by_flavor_id(self.context, 'mainflavor')
         self._test_destroy(flavor)
 
-    def test_destroy_missing_flavor_by_name(self):
-        flavor = objects.Flavor(context=self.context, name='foo')
-        self.assertRaises(exception.FlavorNotFoundByName,
+    def test_destroy_missing_flavor_by_flavorid(self):
+        flavor = objects.Flavor(context=self.context, flavorid='foo')
+        self.assertRaises(exception.FlavorNotFound,
                           flavor.destroy)
 
     def test_destroy_missing_flavor_by_id(self):
-        flavor = objects.Flavor(context=self.context, name='foo', id=1234)
+        flavor = objects.Flavor(context=self.context, flavorid='foo', id=1234)
         self.assertRaises(exception.FlavorNotFound,
                           flavor.destroy)
 
@@ -208,9 +208,7 @@ class FlavorObjectTestCase(test.NoDBTestCase):
         self._test_get_all(expect_len + 1)
 
     def test_get_all_with_all_api_flavors(self):
-        db_flavors = db_api.flavor_get_all(self.context)
-        for flavor in db_flavors:
-            db_api.flavor_destroy(self.context, flavor['name'])
+        self._delete_main_flavors()
         flavor = ForcedFlavor(context=self.context, **fake_api_flavor)
         flavor.create()
         self._test_get_all(1)
diff --git a/nova/tests/unit/api/openstack/compute/test_flavor_manage.py b/nova/tests/unit/api/openstack/compute/test_flavor_manage.py
index 368013ac3d..13baa9aa6b 100644
--- a/nova/tests/unit/api/openstack/compute/test_flavor_manage.py
+++ b/nova/tests/unit/api/openstack/compute/test_flavor_manage.py
@@ -15,6 +15,7 @@
 
 import datetime
 
+import mock
 from oslo_serialization import jsonutils
 import six
 import webob
@@ -52,20 +53,6 @@ def fake_db_flavor(**updates):
     return db_flavor
 
 
-def fake_get_flavor_by_flavor_id(flavorid, ctxt=None, read_deleted='yes'):
-    if flavorid == 'failtest':
-        raise exception.FlavorNotFound(flavor_id=flavorid)
-    elif not str(flavorid) == '1234':
-        raise Exception("This test expects flavorid 1234, not %s" % flavorid)
-    if read_deleted != 'no':
-        raise test.TestingException("Should not be reading deleted")
-    return fake_db_flavor(flavorid=flavorid)
-
-
-def fake_destroy(flavorname):
-    pass
-
-
 def fake_create(newflavor):
     newflavor['flavorid'] = 1234
     newflavor["name"] = 'test'
@@ -86,10 +73,6 @@ class FlavorManageTestV21(test.NoDBTestCase):
 
     def setUp(self):
         super(FlavorManageTestV21, self).setUp()
-        self.stubs.Set(flavors,
-                       "get_flavor_by_flavor_id",
-                       fake_get_flavor_by_flavor_id)
-        self.stubs.Set(flavors, "destroy", fake_destroy)
         self.stub_out("nova.objects.Flavor.create", fake_create)
 
         self.request_body = {
@@ -117,7 +100,8 @@ class FlavorManageTestV21(test.NoDBTestCase):
                                              'os-flavor-access', 'flavors',
                                              'os-flavor-extra-data'))
 
-    def test_delete(self):
+    @mock.patch('nova.objects.Flavor.destroy')
+    def test_delete(self, mock_destroy):
         res = self.controller._delete(self._get_http_request(), 1234)
 
         # NOTE: on v2.1, http status code is set as wsgi_code of API
@@ -130,9 +114,10 @@ class FlavorManageTestV21(test.NoDBTestCase):
         self.assertEqual(202, status_int)
 
         # subsequent delete should fail
+        mock_destroy.side_effect = exception.FlavorNotFound(flavor_id=1234)
         self.assertRaises(webob.exc.HTTPNotFound,
                           self.controller._delete, self._get_http_request(),
-                          "failtest")
+                          1234)
 
     def _test_create_missing_parameter(self, parameter):
         body = {
diff --git a/nova/tests/unit/db/test_db_api.py b/nova/tests/unit/db/test_db_api.py
index bb7a2c9c4e..ca83f510d7 100644
--- a/nova/tests/unit/db/test_db_api.py
+++ b/nova/tests/unit/db/test_db_api.py
@@ -4018,7 +4018,7 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase):
         flavor2 = self._create_flavor({'name': 'name2', 'flavorid': 'a2',
                                        'extra_specs': specs2})
 
-        db.flavor_destroy(self.ctxt, 'name1')
+        db.flavor_destroy(self.ctxt, 'a1')
 
         self.assertRaises(exception.FlavorNotFound,
                           db.flavor_get, self.ctxt, flavor1['id'])
@@ -4067,7 +4067,7 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase):
     def test_flavor_get_all(self):
         # NOTE(boris-42): Remove base instance types
         for it in db.flavor_get_all(self.ctxt):
-            db.flavor_destroy(self.ctxt, it['name'])
+            db.flavor_destroy(self.ctxt, it['flavorid'])
 
         flavors = [
             {'root_gb': 600, 'memory_mb': 100, 'disabled': True,
@@ -4294,7 +4294,7 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase):
     def test_flavor_get_by_flavor_id_deleted(self):
         flavor = self._create_flavor({'name': 'abc', 'flavorid': '123'})
 
-        db.flavor_destroy(self.ctxt, 'abc')
+        db.flavor_destroy(self.ctxt, '123')
 
         flavor_by_fid = db.flavor_get_by_flavor_id(self.ctxt,
                 flavor['flavorid'], read_deleted='yes')
@@ -4306,7 +4306,7 @@ class InstanceTypeTestCase(BaseInstanceTypeTestCase):
         param_dict = {'name': 'abc', 'flavorid': '123'}
 
         self._create_flavor(param_dict)
-        db.flavor_destroy(self.ctxt, 'abc')
+        db.flavor_destroy(self.ctxt, '123')
 
         # Recreate the flavor with the same params
         flavor = self._create_flavor(param_dict)
@@ -4472,13 +4472,13 @@ class InstanceTypeAccessTestCase(BaseInstanceTypeTestCase):
         for v in values:
             self._create_flavor_access(*v)
 
-        db.flavor_destroy(self.ctxt, flavor1['name'])
+        db.flavor_destroy(self.ctxt, flavor1['flavorid'])
 
         p = (self.ctxt, flavor1['flavorid'])
         self.assertEqual(0, len(db.flavor_access_get_by_flavor_id(*p)))
         p = (self.ctxt, flavor2['flavorid'])
         self.assertEqual(1, len(db.flavor_access_get_by_flavor_id(*p)))
-        db.flavor_destroy(self.ctxt, flavor2['name'])
+        db.flavor_destroy(self.ctxt, flavor2['flavorid'])
         self.assertEqual(0, len(db.flavor_access_get_by_flavor_id(*p)))
 
 
diff --git a/nova/tests/unit/objects/test_flavor.py b/nova/tests/unit/objects/test_flavor.py
index cffad0f2ee..408ae0499c 100644
--- a/nova/tests/unit/objects/test_flavor.py
+++ b/nova/tests/unit/objects/test_flavor.py
@@ -353,11 +353,13 @@ class _TestFlavor(object):
         flavor = flavor_obj.Flavor(id=123)
         self.assertRaises(exception.ObjectActionError, flavor.save)
 
-    def test_destroy(self):
-        flavor = flavor_obj.Flavor(context=self.context, name='foo')
+    @mock.patch('nova.objects.Flavor._flavor_destroy')
+    def test_destroy(self, mock_destroy):
+        mock_destroy.side_effect = exception.FlavorNotFound(flavor_id='foo')
+        flavor = flavor_obj.Flavor(context=self.context, flavorid='foo')
         with mock.patch.object(db, 'flavor_destroy') as destroy:
             flavor.destroy()
-            destroy.assert_called_once_with(self.context, flavor.name)
+            destroy.assert_called_once_with(self.context, flavor.flavorid)
 
     @mock.patch('nova.objects.Flavor._flavor_destroy')
     def test_destroy_api_by_id(self, mock_destroy):
@@ -366,10 +368,11 @@ class _TestFlavor(object):
         mock_destroy.assert_called_once_with(self.context, flavor_id=flavor.id)
 
     @mock.patch('nova.objects.Flavor._flavor_destroy')
-    def test_destroy_api_by_name(self, mock_destroy):
-        flavor = flavor_obj.Flavor(context=self.context, name='foo')
+    def test_destroy_api_by_flavorid(self, mock_destroy):
+        flavor = flavor_obj.Flavor(context=self.context, flavorid='foo')
         flavor.destroy()
-        mock_destroy.assert_called_once_with(self.context, name=flavor.name)
+        mock_destroy.assert_called_once_with(self.context,
+                                             flavorid=flavor.flavorid)
 
     def test_load_projects(self):
         flavor = flavor_obj.Flavor(context=self.context, flavorid='foo')
diff --git a/nova/tests/unit/test_flavors.py b/nova/tests/unit/test_flavors.py
index 69f9f0ebfb..0673735cf9 100644
--- a/nova/tests/unit/test_flavors.py
+++ b/nova/tests/unit/test_flavors.py
@@ -61,17 +61,6 @@ DEFAULT_FLAVOR_OBJS = [
 
 class InstanceTypeTestCase(test.TestCase):
     """Test cases for flavor  code."""
-    def test_non_existent_inst_type_should_not_delete(self):
-        # Ensures that flavor creation fails with invalid args.
-        self.assertRaises(exception.FlavorNotFoundByName,
-                          flavors.destroy,
-                          'unknown_flavor')
-
-    def test_will_not_destroy_with_no_name(self):
-        # Ensure destroy said path of no name raises error.
-        self.assertRaises(exception.FlavorNotFoundByName,
-                          flavors.destroy, None)
-
     def test_will_not_get_bad_default_instance_type(self):
         # ensures error raised on bad default flavor.
         self.flags(default_flavor='unknown_flavor')
@@ -432,7 +421,7 @@ class CreateInstanceTypeTest(test.TestCase):
         self.assertNotEqual(len(original_list), len(new_list),
                             'instance type was not created')
 
-        flavors.destroy('flavor')
+        flavor.destroy()
         self.assertRaises(exception.FlavorNotFound,
                           objects.Flavor.get_by_name, ctxt, flavor.name)