From 429775c8e165adffb2bd83e44b6690e5058158d6 Mon Sep 17 00:00:00 2001
From: Adam Gandelman <adamg@ubuntu.com>
Date: Fri, 19 Jun 2015 13:14:40 +0200
Subject: [PATCH] Support string flavor IDs

This allows operating on Nova flavors with non-integer ids
by adding a new str_id field to the view that always contains the
unicode flavor id.  The current id field will now contain a value
only when the flavor's id can be cast to int, otherwise it will be
None.  Validation of flavor id when querying a flavor or creating an
instance has been updated to include non-empty strings in addition to
integers.

This will require a patch to python-troveclient to properly fallback
to str_id in absence of the integer id:

    https://review.openstack.org/#/c/123301/

(cherry picked from commit 21fee8c3c554ba158c2bbe977131522a204c22cc)

Change-Id: Ie9cfefc6127bc76783cdf9668636342d7590b308
Closes-bug: #1333852
---
 .../src/samples/db-flavors-by-id-response-json.txt |  4 +-
 apidocs/src/samples/db-flavors-by-id-response.json |  5 +-
 apidocs/src/samples/db-flavors-response-json.txt   |  4 +-
 apidocs/src/samples/db-flavors-response.json       | 38 +++++++++++---
 .../samples/db-mgmt-get-host-detail-response.json  | 17 ++++++
 etc/tests/localhost.test.conf                      |  8 ++-
 trove/common/apischema.py                          | 21 +++-----
 .../versions/034_change_task_description.py        | 29 +++++++++++
 .../versions/035_flavor_id_int_to_string.py        | 35 +++++++++++++
 trove/flavor/service.py                            |  5 +-
 trove/flavor/views.py                              | 10 +++-
 trove/tests/api/flavors.py                         | 29 +++++++----
 trove/tests/api/instances.py                       | 60 ++++++++++++++++++++++
 trove/tests/api/mgmt/malformed_json.py             |  2 +-
 trove/tests/fakes/nova.py                          | 17 +++---
 .../unittests/instance/test_instance_controller.py | 11 ++--
 16 files changed, 244 insertions(+), 51 deletions(-)
 create mode 100644 apidocs/src/samples/db-mgmt-get-host-detail-response.json
 create mode 100644 trove/db/sqlalchemy/migrate_repo/versions/034_change_task_description.py
 create mode 100644 trove/db/sqlalchemy/migrate_repo/versions/035_flavor_id_int_to_string.py

diff --git a/apidocs/src/samples/db-flavors-by-id-response-json.txt b/apidocs/src/samples/db-flavors-by-id-response-json.txt
index d7fa6a7..27460aa 100644
--- a/apidocs/src/samples/db-flavors-by-id-response-json.txt
+++ b/apidocs/src/samples/db-flavors-by-id-response-json.txt
@@ -1,6 +1,6 @@
 HTTP/1.1 200 OK
 Content-Type: application/json
 Via: 1.1 Repose (Repose/2.6.7)
-Content-Length: 206
+Content-Length: 214
 Date: Mon, 18 Mar 2013 19:09:17 GMT
-Server: Jetty(8.0.y.z-SNAPSHOT)
\ No newline at end of file
+Server: Jetty(8.0.y.z-SNAPSHOT)
diff --git a/apidocs/src/samples/db-flavors-by-id-response.json b/apidocs/src/samples/db-flavors-by-id-response.json
index ec8bee0..de63f57 100644
--- a/apidocs/src/samples/db-flavors-by-id-response.json
+++ b/apidocs/src/samples/db-flavors-by-id-response.json
@@ -12,6 +12,7 @@
             }
         ],
         "name": "512MB Instance",
-        "ram": 512
+        "ram": 512,
+        "str_id": "1"
     }
-}
\ No newline at end of file
+}
diff --git a/apidocs/src/samples/db-flavors-response-json.txt b/apidocs/src/samples/db-flavors-response-json.txt
index 2244dbd..d8f9759 100644
--- a/apidocs/src/samples/db-flavors-response-json.txt
+++ b/apidocs/src/samples/db-flavors-response-json.txt
@@ -1,6 +1,6 @@
 HTTP/1.1 200 OK
 Content-Type: application/json
 Via: 1.1 Repose (Repose/2.6.7)
-Content-Length: 1186
+Content-Length: 2730
 Date: Mon, 18 Mar 2013 19:09:17 GMT
-Server: Jetty(8.0.y.z-SNAPSHOT)
\ No newline at end of file
+Server: Jetty(8.0.y.z-SNAPSHOT)
diff --git a/apidocs/src/samples/db-flavors-response.json b/apidocs/src/samples/db-flavors-response.json
index 222680f..9466447 100644
--- a/apidocs/src/samples/db-flavors-response.json
+++ b/apidocs/src/samples/db-flavors-response.json
@@ -13,7 +13,8 @@
                 }
             ],
             "name": "512MB Instance",
-            "ram": 512
+            "ram": 512,
+            "str_id": "1"
         },
         {
             "id": 2,
@@ -28,7 +29,8 @@
                 }
             ],
             "name": "1GB Instance",
-            "ram": 1024
+            "ram": 1024,
+            "str_id": "2"
         },
         {
             "id": 3,
@@ -43,7 +45,8 @@
                 }
             ],
             "name": "2GB Instance",
-            "ram": 2048
+            "ram": 2048,
+            "str_id": "3"
         },
         {
             "id": 4,
@@ -58,7 +61,8 @@
                 }
             ],
             "name": "4GB Instance",
-            "ram": 4096
+            "ram": 4096,
+            "str_id": "4"
         },
         {
             "id": 5,
@@ -73,10 +77,11 @@
                 }
             ],
             "name": "8GB Instance",
-            "ram": 8192
+            "ram": 8192,
+            "str_id": "5"
         },
         {
-            "id": 6,
+            "id": 5,
             "links": [
                 {
                     "href": "https://ord.databases.api.rackspacecloud.com/v1.0/1234/flavors/6",
@@ -88,7 +93,24 @@
                 }
             ],
             "name": "16GB Instance",
-            "ram": 16384
+            "ram": 16384,
+            "str_id": "6"
+        },
+        {
+            "id": null,
+            "links": [
+                {
+                    "href": "https://troveapi.org/v1.0/1234/flavors/custom",
+                    "rel": "self"
+                },
+                {
+                    "href": "https://troveapi.org/flavors/custom",
+                    "rel": "bookmark"
+                }
+            ],
+            "name": "custom.small",
+            "ram": 512,
+            "str_id": "custom"
         }
     ]
-}
\ No newline at end of file
+}
diff --git a/apidocs/src/samples/db-mgmt-get-host-detail-response.json b/apidocs/src/samples/db-mgmt-get-host-detail-response.json
new file mode 100644
index 0000000..f5b50c0
--- /dev/null
+++ b/apidocs/src/samples/db-mgmt-get-host-detail-response.json
@@ -0,0 +1,17 @@
+{
+    "host": {
+        "instances": [
+            {
+                "id": "44b277eb-39be-4921-be31-3d61b43651d7",
+                "name": "44b277eb-39be-4921-be31-3d61b43651d7-lay",
+                "server_id": "44b277eb-39be-4921-be31-3d61b43651d7",
+                "status": "ACTIVE",
+                "tenant_id": "3000"
+            }
+        ],
+        "name": "hostname_1",
+        "percentUsed": 12,
+        "totalRAM": 32000,
+        "usedRAM": 4096
+    }
+}
diff --git a/etc/tests/localhost.test.conf b/etc/tests/localhost.test.conf
index 72eb8b5..9574b8b 100644
--- a/etc/tests/localhost.test.conf
+++ b/etc/tests/localhost.test.conf
@@ -114,7 +114,13 @@
             "name": "eph.rd-smaller",
             "ram": 768,
             "local_storage": 2
-        }
+        },
+        {
+            "id": "custom",
+            "name": "custom.small",
+            "ram": 512,
+            "local_storage": 1
+         }
 
     ],
     "sentinel": null
diff --git a/trove/common/apischema.py b/trove/common/apischema.py
index 01258b1..e472686 100644
--- a/trove/common/apischema.py
+++ b/trove/common/apischema.py
@@ -30,6 +30,13 @@ boolean_string = {
     "maximum": 1
 }
 
+non_empty_string = {
+    "type": "string",
+    "minLength": 1,
+    "maxLength": 255,
+    "pattern": "^.*[0-9a-zA-Z]+.*$"
+}
+
 configuration_data_types = {
     "type": "string",
     "minLength": 1,
@@ -51,12 +58,7 @@ configuration_non_empty_string = {
 
 flavorref = {
     'oneOf': [
-        url_ref,
-        {
-            "type": "string",
-            "maxLength": 5,
-            "pattern": "[0-9]+"
-        },
+        non_empty_string,
         {
             "type": "integer"
         }]
@@ -75,13 +77,6 @@ volume_size = {
         }]
 }
 
-non_empty_string = {
-    "type": "string",
-    "minLength": 1,
-    "maxLength": 255,
-    "pattern": "^.*[0-9a-zA-Z]+.*$"
-}
-
 host_string = {
     "type": "string",
     "minLength": 1,
diff --git a/trove/db/sqlalchemy/migrate_repo/versions/034_change_task_description.py b/trove/db/sqlalchemy/migrate_repo/versions/034_change_task_description.py
new file mode 100644
index 0000000..e96e9d8
--- /dev/null
+++ b/trove/db/sqlalchemy/migrate_repo/versions/034_change_task_description.py
@@ -0,0 +1,29 @@
+# Copyright 2014 AWCloud
+#
+#    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.
+
+from sqlalchemy.schema import MetaData
+
+from trove.db.sqlalchemy.migrate_repo.schema import String
+from trove.db.sqlalchemy.migrate_repo.schema import Table
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+    instances = Table('instances', meta, autoload=True)
+    instances.c.task_description.alter(type=String(255))
+
+
+def downgrade(migrate_engine):
+    pass
diff --git a/trove/db/sqlalchemy/migrate_repo/versions/035_flavor_id_int_to_string.py b/trove/db/sqlalchemy/migrate_repo/versions/035_flavor_id_int_to_string.py
new file mode 100644
index 0000000..3a40bc8
--- /dev/null
+++ b/trove/db/sqlalchemy/migrate_repo/versions/035_flavor_id_int_to_string.py
@@ -0,0 +1,35 @@
+#
+# Copyright 2014 Hewlett-Packard Development Company, L.P.
+#
+# 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.
+
+from sqlalchemy.schema import MetaData
+
+from trove.db.sqlalchemy.migrate_repo.schema import Integer
+from trove.db.sqlalchemy.migrate_repo.schema import String
+from trove.db.sqlalchemy.migrate_repo.schema import Table
+
+
+meta = MetaData()
+
+
+def upgrade(migrate_engine):
+    meta.bind = migrate_engine
+    instances = Table('instances', meta, autoload=True)
+    instances.c.flavor_id.alter(String(255))
+
+
+def downgrade(migrate_engine):
+    meta.bind = migrate_engine
+    instances = Table('instances', meta, autoload=True)
+    instances.c.flavor_id.alter(Integer())
diff --git a/trove/flavor/service.py b/trove/flavor/service.py
index 6356bdc..4b56506 100644
--- a/trove/flavor/service.py
+++ b/trove/flavor/service.py
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import six
 
 from trove.common import exception
 from trove.common import wsgi
@@ -27,7 +28,7 @@ class FlavorController(wsgi.Controller):
         """Return a single flavor."""
         context = req.environ[wsgi.CONTEXT_KEY]
         self._validate_flavor_id(id)
-        flavor = models.Flavor(context=context, flavor_id=int(id))
+        flavor = models.Flavor(context=context, flavor_id=id)
         # Pass in the request to build accurate links.
         return wsgi.Result(views.FlavorView(flavor, req).data(), 200)
 
@@ -38,6 +39,8 @@ class FlavorController(wsgi.Controller):
         return wsgi.Result(views.FlavorsView(flavors, req).data(), 200)
 
     def _validate_flavor_id(self, id):
+        if isinstance(id, six.string_types):
+            return
         try:
             if int(id) != float(id):
                 raise exception.NotFound(uuid=id)
diff --git a/trove/flavor/views.py b/trove/flavor/views.py
index 43265a8..751247f 100644
--- a/trove/flavor/views.py
+++ b/trove/flavor/views.py
@@ -28,11 +28,19 @@ class FlavorView(object):
 
     def data(self):
 
+        # If the flavor id cannot be cast to an int, we simply return
+        # no id and rely on str_id instead.
+        try:
+            f_id = int(self.flavor.id)
+        except ValueError:
+            f_id = None
+
         flavor = {
-            'id': int(self.flavor.id),
+            'id': f_id,
             'links': self._build_links(),
             'name': self.flavor.name,
             'ram': self.flavor.ram,
+            'str_id': str(self.flavor.id)
         }
 
         if not CONF.trove_volume_support and CONF.device_path is not None:
diff --git a/trove/tests/api/flavors.py b/trove/tests/api/flavors.py
index de7dba9..268bbd9 100644
--- a/trove/tests/api/flavors.py
+++ b/trove/tests/api/flavors.py
@@ -65,6 +65,11 @@ def assert_link_list_is_equal(flavor):
     assert_true(hasattr(flavor, 'links'))
     assert_true(flavor.links)
 
+    if flavor.id:
+        flavor_id = str(flavor.id)
+    else:
+        flavor_id = flavor.str_id
+
     for link in flavor.links:
         href = link['href']
         if "self" in link['rel']:
@@ -74,12 +79,12 @@ def assert_link_list_is_equal(flavor):
             msg = ("REL HREF %s doesn't start with %s" %
                    (href, test_config.dbaas_url))
             assert_true(href.startswith(url), msg)
-            url = os.path.join("flavors", str(flavor.id))
-            msg = "REL HREF %s doesn't end in 'flavors/id'" % href
+            url = os.path.join("flavors", flavor_id)
+            msg = "REL HREF %s doesn't end in '%s'" % (href, url)
             assert_true(href.endswith(url), msg)
         elif "bookmark" in link['rel']:
             base_url = test_config.version_url.replace('http:', 'https:', 1)
-            expected_href = os.path.join(base_url, "flavors", str(flavor.id))
+            expected_href = os.path.join(base_url, "flavors", flavor_id)
             msg = 'bookmark "href" must be %s, not %s' % (expected_href, href)
             assert_equal(href, expected_href, msg)
         else:
@@ -139,26 +144,30 @@ class Flavors(object):
 
     @test
     def test_flavor_list_attrs(self):
-        expected_attrs = ['id', 'name', 'ram', 'links', 'local_storage']
+        allowed_attrs = ['id', 'name', 'ram', 'links', 'local_storage',
+                         'str_id']
         flavors = self.rd_client.flavors.list()
         attrcheck = AttrCheck()
         for flavor in flavors:
             flavor_dict = flavor._info
-            attrcheck.attrs_exist(flavor_dict, expected_attrs,
-                                  msg="Flavors list")
+            attrcheck.attrs_exist(
+                flavor_dict, allowed_attrs,
+                msg="Flavors list")
             attrcheck.links(flavor_dict['links'])
 
     @test
     def test_flavor_get_attrs(self):
-        expected_attrs = ['id', 'name', 'ram', 'links', 'local_storage']
+        allowed_attrs = ['id', 'name', 'ram', 'links', 'local_storage',
+                         'str_id']
         flavor = self.rd_client.flavors.get(1)
         attrcheck = AttrCheck()
         flavor_dict = flavor._info
-        attrcheck.attrs_exist(flavor_dict, expected_attrs,
-                              msg="Flavor Get 1")
+        attrcheck.attrs_exist(
+            flavor_dict, allowed_attrs,
+            msg="Flavor Get 1")
         attrcheck.links(flavor_dict['links'])
 
     @test
     def test_flavor_not_found(self):
         assert_raises(exceptions.NotFound,
-                      self.rd_client.flavors.get, "detail")
+                      self.rd_client.flavors.get, "foo")
diff --git a/trove/tests/api/instances.py b/trove/tests/api/instances.py
index b124cfb..66a568a 100644
--- a/trove/tests/api/instances.py
+++ b/trove/tests/api/instances.py
@@ -381,6 +381,18 @@ class CreateInstanceFail(object):
 
         self.delete_async(result.id)
 
+    def test_create_failure_with_empty_flavor(self):
+        instance_name = "instance-failure-with-empty-flavor"
+        databases = []
+        if VOLUME_SUPPORT:
+            volume = {'size': 1}
+        else:
+            volume = None
+        assert_raises(exceptions.BadRequest, dbaas.instances.create,
+                      instance_name, '',
+                      volume, databases)
+        assert_equal(400, dbaas.last_http_code)
+
     @test(enabled=VOLUME_SUPPORT)
     def test_create_failure_with_empty_volume(self):
         instance_name = "instance-failure-with-no-volume-size"
@@ -692,6 +704,54 @@ class CreateInstance(object):
                 check.volume()
 
 
+@test(depends_on_classes=[InstanceSetup],
+      groups=[GROUP, tests.INSTANCES],
+      runs_after_groups=[tests.PRE_INSTANCES])
+class CreateInstanceFlavors(object):
+    def _result_is_active(self):
+        instance = dbaas.instances.get(self.result.id)
+        if instance.status == "ACTIVE":
+            return True
+        else:
+            # If its not ACTIVE, anything but BUILD must be
+            # an error.
+            assert_equal("BUILD", instance.status)
+            if instance_info.volume is not None:
+                assert_equal(instance.volume.get('used', None), None)
+            return False
+
+    def _delete_async(self, instance_id):
+        dbaas.instances.delete(instance_id)
+        while True:
+            try:
+                dbaas.instances.get(instance_id)
+            except exceptions.NotFound:
+                return True
+            time.sleep(1)
+
+    def _create_with_flavor(self, flavor_id):
+        if not FAKE:
+            raise SkipTest("This test only for fake mode.")
+        instance_name = "instance-with-flavor-%s" % flavor_id
+        databases = []
+        if VOLUME_SUPPORT:
+            volume = {'size': 1}
+        else:
+            volume = None
+        self.result = dbaas.instances.create(instance_name, flavor_id, volume,
+                                             databases)
+        poll_until(self._result_is_active)
+        self._delete_async(self.result.id)
+
+    @test
+    def test_create_with_int_flavor(self):
+        self._create_with_flavor(1)
+
+    @test
+    def test_create_with_str_flavor(self):
+        self._create_with_flavor('custom')
+
+
 @test(depends_on_classes=[InstanceSetup], groups=[GROUP_NEUTRON])
 class CreateInstanceWithNeutron(unittest.TestCase):
     @time_out(TIMEOUT_INSTANCE_CREATE)
diff --git a/trove/tests/api/mgmt/malformed_json.py b/trove/tests/api/mgmt/malformed_json.py
index 4ab6ffe..44c8193 100644
--- a/trove/tests/api/mgmt/malformed_json.py
+++ b/trove/tests/api/mgmt/malformed_json.py
@@ -131,7 +131,7 @@ class MalformedJson(object):
 
         poll_until(_check_instance_status)
         try:
-            self.dbaas.instances.resize_instance(self.instance.id, "bad data")
+            self.dbaas.instances.resize_instance(self.instance.id, "")
         except Exception as e:
             resp, body = self.dbaas.client.last_response
             httpCode = resp.status
diff --git a/trove/tests/fakes/nova.py b/trove/tests/fakes/nova.py
index f9f4804..e98e34f 100644
--- a/trove/tests/fakes/nova.py
+++ b/trove/tests/fakes/nova.py
@@ -64,6 +64,7 @@ class FakeFlavors(object):
         self._add(10, 2, "m1.rd-tiny", 512)
         self._add(11, 0, "eph.rd-tiny", 512, 1)
         self._add(12, 20, "eph.rd-smaller", 768, 2)
+        self._add("custom", 25, "custom.small", 512, 1)
         # self._add(13, 20, "m1.heat", 512)
 
     def _add(self, *args, **kwargs):
@@ -71,7 +72,11 @@ class FakeFlavors(object):
         self.db[new_flavor.id] = new_flavor
 
     def get(self, id):
-        id = int(id)
+        try:
+            id = int(id)
+        except ValueError:
+            pass
+
         if id not in self.db:
             raise nova_exceptions.NotFound(404, "Flavor id not found %s" % id)
         return self.db[id]
@@ -612,7 +617,7 @@ class FakeHost(object):
         """
         self.instances = []
         self.percentUsed = 0
-        self.totalRAM = 2004  # 16384
+        self.totalRAM = 32000  # 16384
         self.usedRAM = 0
         for server in self.servers.list():
             print(server)
@@ -624,11 +629,11 @@ class FakeHost(object):
                 'name': server.name,
                 'status': server.status
             })
-            try:
-                flavor = FLAVORS.get(server.flavor_ref)
-            except ValueError:
-                # Maybe flavor_ref isn't an int?
+            if (str(server.flavor_ref).startswith('http:') or
+               str(server.flavor_ref).startswith('https:')):
                 flavor = FLAVORS.get_by_href(server.flavor_ref)
+            else:
+                flavor = FLAVORS.get(server.flavor_ref)
             ram = flavor.ram
             self.usedRAM += ram
         decimal = float(self.usedRAM) / float(self.totalRAM)
diff --git a/trove/tests/unittests/instance/test_instance_controller.py b/trove/tests/unittests/instance/test_instance_controller.py
index 171caef..fca6321 100644
--- a/trove/tests/unittests/instance/test_instance_controller.py
+++ b/trove/tests/unittests/instance/test_instance_controller.py
@@ -183,6 +183,12 @@ class TestInstanceController(TestCase):
         validator = jsonschema.Draft4Validator(schema)
         self.assertTrue(validator.is_valid(body))
 
+    def test_validate_resize_instance_string(self):
+        body = {"resize": {"flavorRef": 'foo'}}
+        schema = self.controller.get_schema('action', body)
+        validator = jsonschema.Draft4Validator(schema)
+        self.assertTrue(validator.is_valid(body))
+
     def test_validate_resize_instance_empty_url(self):
         body = {"resize": {"flavorRef": ""}}
         schema = self.controller.get_schema('action', body)
@@ -191,10 +197,7 @@ class TestInstanceController(TestCase):
         errors = sorted(validator.iter_errors(body), key=lambda e: e.path)
         self.verify_errors(errors[0].context,
                            ["'' is too short",
-                            "'' does not match 'http[s]?://(?:[a-zA-Z]"
-                            "|[0-9]|[$-_@.&+]|[!*\\\\(\\\\),]"
-                            "|(?:%[0-9a-fA-F][0-9a-fA-F]))+'",
-                            "'' does not match '[0-9]+'",
+                            "'' does not match '^.*[0-9a-zA-Z]+.*$'",
                             "'' is not of type 'integer'"],
                            ["flavorRef", "flavorRef", "flavorRef",
                             "flavorRef"],
-- 
2.1.4

