From 9ece778613d67feff35342aeaa6618aaa5f6fc85 Mon Sep 17 00:00:00 2001 From: Oleksii Chuprykov Date: Fri, 29 Apr 2016 17:03:17 +0300 Subject: [PATCH] Fix cancel update for nova server with defined port This particular patch fixes a behaviour of cancel update for nova server with defined port, so there are no ports manageable by nova. We have these issues while restoring ports after rollback: 1) We doesn't detach any ports from current server, because we doesn't save them to resoruce data. (we store this data after succesfull create of the server) 2) Detaching an interface from current server will fail, if the server will be in building state, so we need to wait until server will be in active or in error state. Refresh ports list to solve problem (1). Wait until nova moves to active/error state to solve (2). A functional test to prove the fix was added. Note, that this test is skipped for convergence engine tests until cancel update will work properly in convergence mode (see bug 1533176). Partial-Bug: #1570908 Change-Id: If6fd916068a425eea6dc795192f286cb5ffcb794 (cherry picked from commit 584efe3329143e28fdb42b5d9496977f5cdf275a) --- .../openstack/nova/server_network_mixin.py | 116 +++++++++--------- heat/tests/openstack/nova/test_server.py | 21 +++- heat_integrationtests/common/test.py | 19 +++ .../functional/test_cancel_update.py | 63 ++++++++++ 4 files changed, 159 insertions(+), 60 deletions(-) create mode 100644 heat_integrationtests/functional/test_cancel_update.py diff --git a/heat/engine/resources/openstack/nova/server_network_mixin.py b/heat/engine/resources/openstack/nova/server_network_mixin.py index 9def1a221..87b5cb42b 100644 --- a/heat/engine/resources/openstack/nova/server_network_mixin.py +++ b/heat/engine/resources/openstack/nova/server_network_mixin.py @@ -13,6 +13,7 @@ import itertools +import eventlet from oslo_log import log as logging from oslo_serialization import jsonutils from oslo_utils import netutils @@ -21,9 +22,7 @@ import retrying from heat.common import exception from heat.common.i18n import _ from heat.common.i18n import _LI - from heat.engine import resource - from heat.engine.resources.openstack.neutron import port as neutron_port LOG = logging.getLogger(__name__) @@ -413,6 +412,46 @@ class ServerNetworkMixin(object): elif not self.is_using_neutron(): self._floating_ip_nova_associate(floating_ip) + @staticmethod + def get_all_ports(server): + return itertools.chain( + server._data_get_ports(), + server._data_get_ports('external_ports') + ) + + def detach_ports(self, server): + existing_server_id = server.resource_id + for port in self.get_all_ports(server): + self.client_plugin().interface_detach( + existing_server_id, port['id']) + try: + if self.client_plugin().check_interface_detach( + existing_server_id, port['id']): + LOG.info(_LI('Detach interface %(port)s successful from ' + 'server %(server)s.') + % {'port': port['id'], + 'server': existing_server_id}) + except retrying.RetryError: + raise exception.InterfaceDetachFailed( + port=port['id'], server=existing_server_id) + + def attach_ports(self, server): + prev_server_id = server.resource_id + + for port in self.get_all_ports(server): + self.client_plugin().interface_attach(prev_server_id, + port['id']) + try: + if self.client_plugin().check_interface_attach( + prev_server_id, port['id']): + LOG.info(_LI('Attach interface %(port)s successful to ' + 'server %(server)s') + % {'port': port['id'], + 'server': prev_server_id}) + except retrying.RetryError: + raise exception.InterfaceAttachFailed( + port=port['id'], server=prev_server_id) + def prepare_ports_for_replace(self): if not self.is_using_neutron(): return @@ -426,21 +465,7 @@ class ServerNetworkMixin(object): for port_type, port in port_data: data[port_type].append({'id': port['id']}) - # detach the ports from the server - server_id = self.resource_id - for port_type, port in port_data: - self.client_plugin().interface_detach(server_id, port['id']) - try: - if self.client_plugin().check_interface_detach( - server_id, port['id']): - LOG.info(_LI('Detach interface %(port)s successful ' - 'from server %(server)s when prepare ' - 'for replace.') - % {'port': port['id'], - 'server': server_id}) - except retrying.RetryError: - raise exception.InterfaceDetachFailed( - port=port['id'], server=server_id) + self.detach_ports(self) def restore_ports_after_rollback(self, convergence): if not self.is_using_neutron(): @@ -460,46 +485,23 @@ class ServerNetworkMixin(object): else: existing_server = self - port_data = itertools.chain( - existing_server._data_get_ports(), - existing_server._data_get_ports('external_ports') - ) - - existing_server_id = existing_server.resource_id - for port in port_data: - # detach the ports from current resource - self.client_plugin().interface_detach( - existing_server_id, port['id']) + # Wait until server will move to active state. We can't + # detach interfaces from server in BUILDING state. + # In case of convergence, the replacement resource may be + # created but never have been worked on because the rollback was + # trigerred or new update was trigerred. + if existing_server.resource_id is not None: try: - if self.client_plugin().check_interface_detach( - existing_server_id, port['id']): - LOG.info(_LI('Detach interface %(port)s successful from ' - 'server %(server)s when restore after ' - 'rollback.') - % {'port': port['id'], - 'server': existing_server_id}) - except retrying.RetryError: - raise exception.InterfaceDetachFailed( - port=port['id'], server=existing_server_id) + while True: + active = self.client_plugin()._check_active( + existing_server.resource_id) + if active: + break + eventlet.sleep(1) + except exception.ResourceInError: + pass - # attach the ports for old resource - prev_port_data = itertools.chain( - prev_server._data_get_ports(), - prev_server._data_get_ports('external_ports')) + self.store_external_ports() + self.detach_ports(existing_server) - prev_server_id = prev_server.resource_id - - for port in prev_port_data: - self.client_plugin().interface_attach(prev_server_id, - port['id']) - try: - if self.client_plugin().check_interface_attach( - prev_server_id, port['id']): - LOG.info(_LI('Attach interface %(port)s successful to ' - 'server %(server)s when restore after ' - 'rollback.') - % {'port': port['id'], - 'server': prev_server_id}) - except retrying.RetryError: - raise exception.InterfaceAttachFailed( - port=port['id'], server=prev_server_id) + self.attach_ports(prev_server) diff --git a/heat/tests/openstack/nova/test_server.py b/heat/tests/openstack/nova/test_server.py index a0461df65..97258f87e 100644 --- a/heat/tests/openstack/nova/test_server.py +++ b/heat/tests/openstack/nova/test_server.py @@ -35,6 +35,7 @@ from heat.engine.clients.os import zaqar from heat.engine import environment from heat.engine import resource from heat.engine.resources.openstack.nova import server as servers +from heat.engine.resources.openstack.nova import server_network_mixin from heat.engine.resources import scheduler_hints as sh from heat.engine import scheduler from heat.engine import stack as parser @@ -4395,7 +4396,9 @@ class ServerInternalPortTest(common.HeatTestCase): mock.call('test_server', 3344), mock.call('test_server', 5566)]) - def test_restore_ports_after_rollback(self): + @mock.patch.object(server_network_mixin.ServerNetworkMixin, + 'store_external_ports') + def test_restore_ports_after_rollback(self, store_ports): t, stack, server = self._return_template_stack_and_rsrc_defn( 'test', tmpl_server_with_network_id) server.resource_id = 'existing_server' @@ -4403,6 +4406,8 @@ class ServerInternalPortTest(common.HeatTestCase): external_port_ids = [{'id': 5566}] server._data = {"internal_ports": jsonutils.dumps(port_ids), "external_ports": jsonutils.dumps(external_port_ids)} + self.patchobject(nova.NovaClientPlugin, '_check_active') + nova.NovaClientPlugin._check_active.side_effect = [False, True] # add data to old server in backup stack old_server = mock.Mock() @@ -4420,6 +4425,8 @@ class ServerInternalPortTest(common.HeatTestCase): server.restore_prev_rsrc() + self.assertEqual(2, nova.NovaClientPlugin._check_active.call_count) + # check, that ports were detached from new server nova.NovaClientPlugin.interface_detach.assert_has_calls([ mock.call('existing_server', 1122), @@ -4432,12 +4439,16 @@ class ServerInternalPortTest(common.HeatTestCase): mock.call('old_server', 3344), mock.call('old_server', 5566)]) - def test_restore_ports_after_rollback_attach_failed(self): + @mock.patch.object(server_network_mixin.ServerNetworkMixin, + 'store_external_ports') + def test_restore_ports_after_rollback_attach_failed(self, store_ports): t, stack, server = self._return_template_stack_and_rsrc_defn( 'test', tmpl_server_with_network_id) server.resource_id = 'existing_server' port_ids = [{'id': 1122}, {'id': 3344}] server._data = {"internal_ports": jsonutils.dumps(port_ids)} + self.patchobject(nova.NovaClientPlugin, '_check_active') + nova.NovaClientPlugin._check_active.return_value = True # add data to old server in backup stack old_server = mock.Mock() @@ -4465,10 +4476,14 @@ class ServerInternalPortTest(common.HeatTestCase): '(old_server)', six.text_type(exc)) - def test_restore_ports_after_rollback_convergence(self): + @mock.patch.object(server_network_mixin.ServerNetworkMixin, + 'store_external_ports') + def test_restore_ports_after_rollback_convergence(self, store_ports): t = template_format.parse(tmpl_server_with_network_id) stack = utils.parse_stack(t) stack.store() + self.patchobject(nova.NovaClientPlugin, '_check_active') + nova.NovaClientPlugin._check_active.return_value = True # mock resource from previous template prev_rsrc = stack['server'] diff --git a/heat_integrationtests/common/test.py b/heat_integrationtests/common/test.py index a7aec6c47..b9bb0362c 100644 --- a/heat_integrationtests/common/test.py +++ b/heat_integrationtests/common/test.py @@ -411,6 +411,25 @@ class HeatIntegrationTest(testscenarios.WithScenarios, self._wait_for_stack_status(**kwargs) + def cancel_update_stack(self, stack_identifier, + expected_status='ROLLBACK_COMPLETE'): + + stack_name = stack_identifier.split('/')[0] + + self.updated_time[stack_identifier] = self.client.stacks.get( + stack_identifier, resolve_outputs=False).updated_time + + self.client.actions.cancel_update(stack_name) + + kwargs = {'stack_identifier': stack_identifier, + 'status': expected_status} + if expected_status in ['ROLLBACK_COMPLETE']: + # To trigger rollback you would intentionally fail the stack + # Hence check for rollback failures + kwargs['failure_pattern'] = '^ROLLBACK_FAILED$' + + self._wait_for_stack_status(**kwargs) + def preview_update_stack(self, stack_identifier, template, environment=None, files=None, parameters=None, tags=None, disable_rollback=True, diff --git a/heat_integrationtests/functional/test_cancel_update.py b/heat_integrationtests/functional/test_cancel_update.py new file mode 100644 index 000000000..c40aeb0a0 --- /dev/null +++ b/heat_integrationtests/functional/test_cancel_update.py @@ -0,0 +1,63 @@ +# 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 heat_integrationtests.functional import functional_base + + +class CancelUpdateTest(functional_base.FunctionalTestsBase): + + template = ''' +heat_template_version: '2013-05-23' +parameters: + InstanceType: + type: string + ImageId: + type: string + network: + type: string +resources: + port: + type: OS::Neutron::Port + properties: + network: {get_param: network} + Server: + type: OS::Nova::Server + properties: + flavor_update_policy: REPLACE + image: {get_param: ImageId} + flavor: {get_param: InstanceType} + networks: + - port: {get_resource: port} +''' + + def setUp(self): + super(CancelUpdateTest, self).setUp() + if not self.conf.image_ref: + raise self.skipException("No image configured to test.") + if not self.conf.instance_type: + raise self.skipException("No flavor configured to test.") + if not self.conf.minimal_instance_type: + raise self.skipException("No minimal flavor configured to test.") + + def test_cancel_update_server_with_port(self): + parameters = {'InstanceType': self.conf.minimal_instance_type, + 'ImageId': self.conf.image_ref, + 'network': self.conf.fixed_network_name} + + stack_identifier = self.stack_create(template=self.template, + parameters=parameters) + parameters['InstanceType'] = 'm1.large' + self.update_stack(stack_identifier, self.template, + parameters=parameters, + expected_status='UPDATE_IN_PROGRESS') + + self.cancel_update_stack(stack_identifier)