Merge "Shorten physical resource name to custom limit" into stable/havana

9 files changed, 136 insertions, 68 deletions

diff --git a/heat/engine/resource.py b/heat/engine/resource.py
index df076e3c3..1141b008c 100644
--- a/heat/engine/resource.py
+++ b/heat/engine/resource.py
@@ -120,6 +120,10 @@ class Resource(object):
     # throughout its lifecycle
     requires_deferred_auth = False
 
+    # Limit to apply to physical_resource_name() size reduction algorithm.
+    # If set to None no limit will be applied.
+    physical_resource_name_limit = 255
+
     def __new__(cls, name, json, stack):
         '''Create a new Resource of the appropriate class for its type.'''
 
@@ -479,10 +483,38 @@ class Resource(object):
         if self.id is None:
             return None
 
-        return '%s-%s-%s' % (self.stack.name,
+        name = '%s-%s-%s' % (self.stack.name,
                              self.name,
                              short_id.get_id(self.id))
 
+        if self.physical_resource_name_limit:
+            name = self.reduce_physical_resource_name(
+                name, self.physical_resource_name_limit)
+        return name
+
+    @staticmethod
+    def reduce_physical_resource_name(name, limit):
+        '''
+        Reduce length of physical resource name to a limit.
+
+        The reduced name will consist of the following:
+        * the first 2 characters of the name
+        * a hyphen
+        * the end of the name, truncated on the left to bring
+          the name length within the limit
+        :param name: The name to reduce the length of
+        :param limit:
+        :returns: A name whose length is less than or equal to the limit
+        '''
+        if len(name) <= limit:
+            return name
+
+        if limit < 4:
+            raise ValueError(_('limit cannot be less than 4'))
+
+        postfix_length = limit - 3
+        return name[0:2] + '-' + name[-postfix_length:]
+
     def validate(self):
         logger.info('Validating %s' % str(self))
 
diff --git a/heat/engine/resources/instance.py b/heat/engine/resources/instance.py
index c4226f8e2..dd36d648f 100644
--- a/heat/engine/resources/instance.py
+++ b/heat/engine/resources/instance.py
@@ -180,6 +180,10 @@ class Instance(resource.Resource):
     update_allowed_keys = ('Metadata', 'Properties')
     update_allowed_properties = ('InstanceType',)
 
+    # Server host name limit to 53 characters by due to typical default
+    # linux HOST_NAME_MAX of 64, minus the .novalocal appended to the name
+    physical_resource_name_limit = 53
+
     def __init__(self, name, json_snippet, stack):
         super(Instance, self).__init__(name, json_snippet, stack)
         self.ipaddress = None
@@ -336,18 +340,6 @@ class Instance(resource.Resource):
                                 subnet_id=self.properties['SubnetId'])
         server = None
 
-        # TODO(sdake/shardy) ensure physical_resource_name() never returns a
-        # string longer than 63 characters, as this is pretty inconvenient
-        # behavior for autoscaling groups and nested stacks where instance
-        # names can easily become quite long even with terse names.
-        physical_resource_name_len = len(self.physical_resource_name())
-        if physical_resource_name_len > 63:
-            raise exception.Error(_('Server %(server)s length %(length)d > 63'
-                                  ' characters, please reduce the length of'
-                                  ' stack or resource names') %
-                                  dict(server=self.physical_resource_name(),
-                                       length=physical_resource_name_len))
-
         try:
             server = self.nova().servers.create(
                 name=self.physical_resource_name(),
diff --git a/heat/engine/resources/server.py b/heat/engine/resources/server.py
index 0b9981291..6b2dc99e1 100644
--- a/heat/engine/resources/server.py
+++ b/heat/engine/resources/server.py
@@ -101,7 +101,7 @@ class Server(resource.Resource):
                              'placement')},
         'security_groups': {
             'Type': 'List',
-            'Description': _('List of security group names')},
+            'Description': _('List of security group names or IDs.')},
         'networks': {
             'Type': 'List',
             'Description': _('An ordered list of nics to be '
@@ -165,6 +165,10 @@ class Server(resource.Resource):
     update_allowed_keys = ('Metadata', 'Properties')
     update_allowed_properties = ('flavor', 'flavor_update_policy')
 
+    # Server host name limit to 53 characters by due to typical default
+    # linux HOST_NAME_MAX of 64, minus the .novalocal appended to the name
+    physical_resource_name_limit = 53
+
     def __init__(self, name, json_snippet, stack):
         super(Server, self).__init__(name, json_snippet, stack)
         self.mime_string = None
@@ -206,18 +210,6 @@ class Server(resource.Resource):
         config_drive = self.properties.get('config_drive')
         disk_config = self.properties.get('diskConfig')
 
-        # TODO(sdake/shardy) ensure physical_resource_name() never returns a
-        # string longer than 63 characters, as this is pretty inconvenient
-        # behavior for autoscaling groups and nested stacks where instance
-        # names can easily become quite long even with terse names.
-        physical_resource_name_len = len(self.physical_resource_name())
-        if physical_resource_name_len > 63:
-            raise exception.Error(_('Server %(server)s length %(length)d > 63'
-                                  ' characters, please reduce the length of'
-                                  ' stack or resource names') %
-                                  dict(server=self.physical_resource_name(),
-                                       length=physical_resource_name_len))
-
         server = None
         try:
             server = self.nova().servers.create(
diff --git a/heat/tests/test_instance.py b/heat/tests/test_instance.py
index 9d3b29674..ced4b24de 100644
--- a/heat/tests/test_instance.py
+++ b/heat/tests/test_instance.py
@@ -98,7 +98,10 @@ class InstancesTest(HeatTestCase):
             self.m.StubOutWithMock(self.fc.servers, 'create')
             self.fc.servers.create(
                 image=1, flavor=1, key_name='test',
-                name=utils.PhysName(stack_name, instance.name),
+                name=utils.PhysName(
+                    stack_name,
+                    instance.name,
+                    limit=instance.physical_resource_name_limit),
                 security_groups=None,
                 userdata=server_userdata, scheduler_hints=None,
                 meta=None, nics=None, availability_zone=None).AndReturn(
@@ -266,21 +269,6 @@ class InstancesTest(HeatTestCase):
 
         self.m.VerifyAll()
 
-    def test_instance_create_err_toolong(self):
-        # Attempt to create a server with a 64 character name should fail
-        # instance name is name_s-name-xxxxxxxxxxxx, so 24 characters gives
-        # a 64 character physical_resource_name
-        return_server = self.fc.servers.list()[1]
-        name = 'e' * 24
-        error = self.assertRaises(exception.ResourceFailure,
-                                  self._create_test_instance,
-                                  return_server,
-                                  name, stub_create=False)
-        substr = ('length 64 > 63 characters, '
-                  'please reduce the length of stack or resource names')
-        self.assertIn(substr, str(error))
-        self.m.VerifyAll()
-
     def test_instance_validate(self):
         stack_name = 'test_instance_validate_stack'
         (t, stack) = self._setup_test_stack(stack_name)
diff --git a/heat/tests/test_loadbalancer.py b/heat/tests/test_loadbalancer.py
index 919a06a75..843edb27f 100644
--- a/heat/tests/test_loadbalancer.py
+++ b/heat/tests/test_loadbalancer.py
@@ -132,9 +132,10 @@ class LoadBalancerTest(HeatTestCase):
         self.m.StubOutWithMock(wc.WaitConditionHandle, 'keystone')
         wc.WaitConditionHandle.keystone().MultipleTimes().AndReturn(self.fkc)
 
-        server_name = utils.PhysName(utils.PhysName('test_stack',
-                                                    'LoadBalancer'),
-                                     'LB_instance')
+        server_name = utils.PhysName(
+            utils.PhysName('test_stack', 'LoadBalancer'),
+            'LB_instance',
+            limit=instance.Instance.physical_resource_name_limit)
         clients.OpenStackClients.nova(
             "compute").MultipleTimes().AndReturn(self.fc)
         self.fc.servers.create(
diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py
index 588c6f5ac..e08b0a34e 100644
--- a/heat/tests/test_resource.py
+++ b/heat/tests/test_resource.py
@@ -14,6 +14,8 @@
 
 import itertools
 
+import testscenarios
+
 from heat.common import exception
 from heat.engine import dependencies
 from heat.engine import parser
@@ -29,6 +31,9 @@ from heat.tests.common import HeatTestCase
 from heat.tests import utils
 
 
+load_tests = testscenarios.load_tests_apply_scenarios
+
+
 class ResourceTest(HeatTestCase):
     def setUp(self):
         super(ResourceTest, self).setUp()
@@ -1034,3 +1039,70 @@ class MetadataTest(HeatTestCase):
         test_data = {'Test': 'Newly-written data'}
         self.res.metadata = test_data
         self.assertEqual(self.res.metadata, test_data)
+
+
+class ReducePhysicalResourceNameTest(HeatTestCase):
+    scenarios = [
+        ('one', dict(
+            limit=10,
+            original='one',
+            reduced='one')),
+        ('limit_plus_one', dict(
+            will_reduce=True,
+            limit=10,
+            original='onetwothree',
+            reduced='on-wothree')),
+        ('limit_exact', dict(
+            limit=11,
+            original='onetwothree',
+            reduced='onetwothree')),
+        ('limit_minus_one', dict(
+            limit=12,
+            original='onetwothree',
+            reduced='onetwothree')),
+        ('limit_four', dict(
+            will_reduce=True,
+            limit=4,
+            original='onetwothree',
+            reduced='on-e')),
+        ('limit_three', dict(
+            will_raise=ValueError,
+            limit=3,
+            original='onetwothree')),
+        ('three_nested_stacks', dict(
+            will_reduce=True,
+            limit=63,
+            original=('ElasticSearch-MasterCluster-ccicxsm25ug6-MasterSvr1'
+                      '-men65r4t53hh-MasterServer-gxpc3wqxy4el'),
+            reduced=('El-icxsm25ug6-MasterSvr1-men65r4t53hh-'
+                     'MasterServer-gxpc3wqxy4el'))),
+        ('big_names', dict(
+            will_reduce=True,
+            limit=63,
+            original=('MyReallyQuiteVeryLongStackName-'
+                      'MyExtraordinarilyLongResourceName-ccicxsm25ug6'),
+            reduced=('My-LongStackName-'
+                     'MyExtraordinarilyLongResourceName-ccicxsm25ug6'))),
+    ]
+
+    will_raise = None
+
+    will_reduce = False
+
+    def test_reduce(self):
+        if self.will_raise:
+            self.assertRaises(
+                self.will_raise,
+                resource.Resource.reduce_physical_resource_name,
+                self.original,
+                self.limit)
+        else:
+            reduced = resource.Resource.reduce_physical_resource_name(
+                self.original, self.limit)
+            self.assertEqual(self.reduced, reduced)
+            if self.will_reduce:
+                # check it has been truncated to exactly the limit
+                self.assertEqual(len(reduced), self.limit)
+            else:
+                # check that nothing has changed
+                self.assertEqual(self.original, reduced)
diff --git a/heat/tests/test_server.py b/heat/tests/test_server.py
index cae806615..686384b28 100644
--- a/heat/tests/test_server.py
+++ b/heat/tests/test_server.py
@@ -142,21 +142,6 @@ class ServersTest(HeatTestCase):
         self.assertEqual('::babe:4317:0A83', server.FnGetAtt('accessIPv6'))
         self.m.VerifyAll()
 
-    def test_server_create_err_toolong(self):
-        # Attempt to create a server with a 64 character name should fail
-        # instance name is name_s-name-xxxxxxxxxxxx, so 24 characters gives
-        # a 64 character physical_resource_name
-        return_server = self.fc.servers.list()[1]
-        name = 'e' * 24
-        error = self.assertRaises(exception.ResourceFailure,
-                                  self._create_test_server,
-                                  return_server,
-                                  name, stub_create=False)
-        substr = ('length 64 > 63 characters, '
-                  'please reduce the length of stack or resource names')
-        self.assertIn(substr, str(error))
-        self.m.VerifyAll()
-
     def test_server_create_with_image_id(self):
         return_server = self.fc.servers.list()[1]
         server = self._setup_test_server(return_server,
diff --git a/heat/tests/utils.py b/heat/tests/utils.py
index 6aa2db0a8..8dd56ad16 100644
--- a/heat/tests/utils.py
+++ b/heat/tests/utils.py
@@ -24,6 +24,7 @@ from heat.common import context
 from heat.common import exception
 from heat.engine import environment
 from heat.engine import parser
+from heat.engine import resource
 
 from heat.db.sqlalchemy.session import get_engine
 from heat.db import migration
@@ -152,9 +153,13 @@ def parse_stack(t, params={}, stack_name='test_stack', stack_id=None):
 
 
 class PhysName(object):
-    def __init__(self, stack_name, resource_name):
+
+    mock_short_id = 'x' * 12
+
+    def __init__(self, stack_name, resource_name, limit=255):
         self.stack_name = stack_name
         self.resource_name = resource_name
+        self.limit = limit
 
     def __eq__(self, physical_name):
         try:
@@ -162,18 +167,18 @@ class PhysName(object):
         except ValueError:
             return False
 
-        if self.stack_name != stack or self.resource_name != res:
-            return False
-
-        if len(short_id) != 12:
+        if len(short_id) != len(self.mock_short_id):
             return False
 
-        return True
+        # ignore the stack portion of the name, as it may have been truncated
+        return self.resource_name == res
 
     def __ne__(self, physical_name):
         return not self.__eq__(physical_name)
 
     def __repr__(self):
-        return '%s-%s-%s' % (self.stack_name,
+        name = '%s-%s-%s' % (self.stack_name,
                              self.resource_name,
-                             'x' * 12)
+                             self.mock_short_id)
+        return resource.Resource.reduce_physical_resource_name(
+            name, self.limit)
diff --git a/test-requirements.txt b/test-requirements.txt
index 80326c3f5..cd727dff2 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -10,6 +10,7 @@ mock>=1.0
 mox>=0.5.3
 testtools>=0.9.32
 testrepository>=0.0.17
+testscenarios>=0.4
 python-glanceclient>=0.9.0
 sphinx>=1.1.2,<1.2
 oslo.sphinx