From 006b95949ca532abe6a4d2de2da144e995190452 Mon Sep 17 00:00:00 2001 From: Ethan Lynn Date: Fri, 22 Aug 2014 13:50:36 +0800 Subject: [PATCH] Prevent excessive validation for maxPersonality limit We have PERSONALITY property on OS::Nova::Server that defaults to {}, but code that triggers check for limits compares value of property to None -> this property always get validated for limits (if limit is set). Change-Id: I552a03cd9f4ab570aa059349ab361bb50c9c0d1e Closes-bug: #1333283 (cherry picked from commit 78b68ba0da5d6ad71397538e484fb7565333129b) --- heat/engine/resources/server.py | 4 ++-- heat/tests/test_server.py | 39 +++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/heat/engine/resources/server.py b/heat/engine/resources/server.py index 4f5f45ced9..bfbc2518bf 100644 --- a/heat/engine/resources/server.py +++ b/heat/engine/resources/server.py @@ -929,7 +929,7 @@ class Server(stack_user.StackUser): # retrieve provider's absolute limits if it will be needed metadata = self.properties.get(self.METADATA) personality = self._personality() - if metadata is not None or personality is not None: + if metadata is not None or personality: limits = nova_utils.absolute_limits(self.nova()) # if 'security_groups' present for the server and explict 'port' @@ -951,7 +951,7 @@ class Server(stack_user.StackUser): # verify the number of personality files and the size of each # personality file against the provider's absolute limits - if personality is not None: + if personality: msg = _("The personality property may not contain " "greater than %s entries.") % limits['maxPersonality'] self._check_maximum(len(personality), diff --git a/heat/tests/test_server.py b/heat/tests/test_server.py index aad04dbfd3..504777d120 100644 --- a/heat/tests/test_server.py +++ b/heat/tests/test_server.py @@ -729,8 +729,6 @@ class ServersTest(HeatTestCase): server = servers.Server('server_create_image_err', t['Resources']['WebServer'], stack) - self.m.StubOutWithMock(server, 'nova') - server.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(clients.OpenStackClients, 'nova') clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(image.ImageConstraint, "validate") @@ -751,7 +749,7 @@ class ServersTest(HeatTestCase): web_server = t['Resources']['WebServer'] del web_server['Properties']['image'] - def create_server(device_name, mock_nova=True): + def create_server(device_name): self.m.UnsetStubs() web_server['Properties']['block_device_mapping'] = [{ "device_name": device_name, @@ -760,9 +758,6 @@ class ServersTest(HeatTestCase): }] server = servers.Server('server_with_bootable_volume', web_server, stack) - if mock_nova: - self.m.StubOutWithMock(server, 'nova') - server.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(clients.OpenStackClients, 'nova') clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) self.m.ReplayAll() @@ -772,7 +767,7 @@ class ServersTest(HeatTestCase): self.assertIsNone(server.validate()) server = create_server('vda') self.assertIsNone(server.validate()) - server = create_server('vdb', mock_nova=False) + server = create_server('vdb') ex = self.assertRaises(exception.StackValidationFailed, server.validate) self.assertEqual('Neither image nor bootable volume is specified for ' @@ -813,7 +808,6 @@ class ServersTest(HeatTestCase): t['Resources']['WebServer'], stack) self.m.StubOutWithMock(server, 'nova') - server.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(image.ImageConstraint, "validate") image.ImageConstraint.validate( mox.IgnoreArg(), mox.IgnoreArg()).MultipleTimes().AndReturn(True) @@ -906,8 +900,6 @@ class ServersTest(HeatTestCase): server = servers.Server('server_validate_net_security_groups', t['Resources']['WebServer'], stack) - self.m.StubOutWithMock(server, 'nova') - server.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(clients.OpenStackClients, 'nova') clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) self.m.ReplayAll() @@ -1682,8 +1674,6 @@ class ServersTest(HeatTestCase): server = servers.Server('server_create_image_err', t['Resources']['WebServer'], stack) - self.m.StubOutWithMock(server, 'nova') - server.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(clients.OpenStackClients, 'nova') clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) self.m.ReplayAll() @@ -1700,8 +1690,6 @@ class ServersTest(HeatTestCase): server = servers.Server('server_create_image_err', t['Resources']['WebServer'], stack) - self.m.StubOutWithMock(server, 'nova') - server.nova().MultipleTimes().AndReturn(self.fc) self.m.StubOutWithMock(clients.OpenStackClients, 'nova') clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) self.m.ReplayAll() @@ -2428,6 +2416,29 @@ class ServersTest(HeatTestCase): self.assertEqual((server.UPDATE, server.COMPLETE), server.state) self.m.VerifyAll() + def test_server_dont_validate_personality_if_personality_isnt_set(self): + stack_name = 'srv_val' + (tmpl, stack) = self._setup_test_stack(stack_name) + + server = servers.Server('server_create_image_err', + tmpl['Resources']['WebServer'], stack) + + # We mock out nova_utils.absolute_limits but we don't specify + # how this mock should behave, so mox will verify that this mock + # is NOT called during call to server.validate(). + # This is the way to validate that no excessive calls to Nova + # are made during validation. + self.m.StubOutWithMock(nova_utils, 'absolute_limits') + self.m.StubOutWithMock(clients.OpenStackClients, 'nova') + clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) + self.m.ReplayAll() + + # Assert here checks that server resource validates, but actually + # this call is Act stage of this test. We calling server.validate() + # to verify that no excessive calls to Nova are made during validation. + self.assertIsNone(server.validate()) + self.m.VerifyAll() + class FlavorConstraintTest(HeatTestCase):