Fix prepare_for_replace/restore_prev_rsrc handing for server

Now, we set 'fixed_ips' to [] for server ports when prepare
for server replacement, but the ports are still in-use if only
set 'fixed_ips' to []. So this patch will to detach the ports from
nova server to make sure same ports can be attached to new one in
prepare_for_replace(). Also, when restoring server, we need to detach
ports from existing server, and then to attach them to previous
server.

We check the interface attach/detach complete by list the
server.interfaces, this change will use 'retry' wrapper to re-poll
the server interfaces for several times (by new config option
'max_interface_check_attempts', default is 10), then will raise
exception if the attach/detach still not complete.

Closes-Bug: #1533076
Change-Id: I7b322f9cf16c100dcd0365bc3091c289f00f0548

heat/common/config.py

@@ -131,6 +131,11 @@ engine_opts = [
                help=_('Number of times to retry when a client encounters an '
                       'expected intermittent error. Set to 0 to disable '
                       'retries.')),
+    cfg.IntOpt('max_interface_check_attempts',
+               min=1,
+               default=10,
+               help=_('Number of times to check whether an interface has '
+                      'been attached or detached.')),
     cfg.IntOpt('event_purge_batch_size',
                default=10,
                help=_("Controls how many events will be pruned whenever a "

heat/common/exception.py

@@ -451,6 +451,16 @@ class EventSendFailed(HeatException):
                 "on other engine (%(engine_id)s)")
 
 
+class InterfaceAttachFailed(HeatException):
+    msg_fmt = _("Failed to attach interface (%(port)s) "
+                "to server (%(server)s)")
+
+
+class InterfaceDetachFailed(HeatException):
+    msg_fmt = _("Failed to detach interface (%(port)s) "
+                "from server (%(server)s)")
+
+
 class UnsupportedObjectError(HeatException):
     msg_fmt = _('Unsupported object type %(objtype)s')
 

heat/engine/clients/client_plugin.py

@@ -289,3 +289,7 @@ class ClientPlugin(object):
 
 def retry_if_connection_err(exception):
     return isinstance(exception, requests.ConnectionError)
+
+
+def retry_if_result_is_false(result):
+    return result is False

heat/engine/clients/os/nova.py

@@ -687,6 +687,30 @@ echo -e '%s\tALL=(ALL)\tNOPASSWD: ALL' >> /etc/sudoers
         else:
             return False
 
+    @retry(stop_max_attempt_number=cfg.CONF.max_interface_check_attempts,
+           wait_fixed=500,
+           retry_on_result=client_plugin.retry_if_result_is_false)
+    def check_interface_detach(self, server_id, port_id):
+        server = self.fetch_server(server_id)
+        if server:
+            interfaces = server.interface_list()
+            for iface in interfaces:
+                if iface.port_id == port_id:
+                    return False
+        return True
+
+    @retry(stop_max_attempt_number=cfg.CONF.max_interface_check_attempts,
+           wait_fixed=500,
+           retry_on_result=client_plugin.retry_if_result_is_false)
+    def check_interface_attach(self, server_id, port_id):
+        server = self.fetch_server(server_id)
+        if server:
+            interfaces = server.interface_list()
+            for iface in interfaces:
+                if iface.port_id == port_id:
+                    return True
+        return False
+
     @os_client.MEMOIZE_EXTENSIONS
     def _list_extensions(self):
         extensions = self.client().list_extensions.show_all()

heat/engine/resources/openstack/nova/server_network_mixin.py

@@ -13,15 +13,21 @@
 
 import itertools
 
+from oslo_log import log as logging
 from oslo_serialization import jsonutils
 from oslo_utils import netutils
+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__)
+
 
 class ServerNetworkMixin(object):
 
@@ -195,7 +201,6 @@ class ServerNetworkMixin(object):
     def _build_nics(self, networks):
         if not networks:
             return None
-
         nics = []
 
         for idx, net in enumerate(networks):
@@ -414,22 +419,23 @@ class ServerNetworkMixin(object):
             [('external_ports', port)
              for port in self._data_get_ports('external_ports')]))
         for port_type, port in port_data:
-            # store port fixed_ips for restoring after failed update
-            port_details = self.client('neutron').show_port(port['id'])['port']
-            fixed_ips = port_details.get('fixed_ips', [])
-            data[port_type].append({'id': port['id'], 'fixed_ips': fixed_ips})
+            data[port_type].append({'id': port['id']})
 
-        if data.get('internal_ports'):
-            self.data_set('internal_ports',
-                          jsonutils.dumps(data['internal_ports']))
-        if data.get('external_ports'):
-            self.data_set('external_ports',
-                          jsonutils.dumps(data['external_ports']))
-        # reset fixed_ips for these ports by setting for each of them
-        # fixed_ips to []
+        # detach the ports from the server
+        server_id = self.resource_id
         for port_type, port in port_data:
-            self.client('neutron').update_port(
-                port['id'], {'port': {'fixed_ips': []}})
+            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)
 
     def restore_ports_after_rollback(self, convergence):
         if not self.is_using_neutron():
@@ -453,16 +459,42 @@ class ServerNetworkMixin(object):
             existing_server._data_get_ports(),
             existing_server._data_get_ports('external_ports')
         )
-        for port in port_data:
-            # reset fixed_ips to [] for new resource
-            self.client('neutron').update_port(port['id'],
-                                               {'port': {'fixed_ips': []}})
 
-        # restore ip for old port
+        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'])
+            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)
+
+        # attach the ports for old resource
         prev_port_data = itertools.chain(
             prev_server._data_get_ports(),
             prev_server._data_get_ports('external_ports'))
+
+        prev_server_id = prev_server.resource_id
+
         for port in prev_port_data:
-            fixed_ips = port['fixed_ips']
-            self.client('neutron').update_port(
-                port['id'], {'port': {'fixed_ips': fixed_ips}})
+            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)

heat/tests/openstack/nova/test_server.py

@@ -4266,8 +4266,6 @@ class ServerInternalPortTest(common.HeatTestCase):
                                             'delete_port')
         self.port_show = self.patchobject(neutronclient.Client,
                                           'show_port')
-        self.port_update = self.patchobject(neutronclient.Client,
-                                            'update_port')
 
     def _return_template_stack_and_rsrc_defn(self, stack_name, temp):
         templ = template.Template(template_format.parse(temp),
@@ -4822,105 +4820,118 @@ class ServerInternalPortTest(common.HeatTestCase):
         self.assertEqual({'port_type': 'external_ports'},
                          update_data.call_args_list[1][1])
 
+    def test_prepare_ports_for_replace_detach_failed(self):
+        t, stack, server = self._return_template_stack_and_rsrc_defn(
+            'test', tmpl_server_with_network_id)
+
+        class Fake(object):
+            def interface_list(self):
+                return [iface(1122)]
+        iface = collections.namedtuple('iface', ['port_id'])
+
+        server.resource_id = 'ser-11'
+        port_ids = [{'id': 1122}]
+
+        server._data = {"internal_ports": jsonutils.dumps(port_ids)}
+        self.patchobject(nova.NovaClientPlugin, 'interface_detach')
+        self.patchobject(nova.NovaClientPlugin, 'fetch_server')
+        nova.NovaClientPlugin.fetch_server.side_effect = [Fake()] * 10
+
+        exc = self.assertRaises(exception.InterfaceDetachFailed,
+                                server.prepare_for_replace)
+        self.assertIn('Failed to detach interface (1122) from server '
+                      '(ser-11)',
+                      six.text_type(exc))
+
     def test_prepare_ports_for_replace(self):
         t, stack, server = self._return_template_stack_and_rsrc_defn(
             'test', tmpl_server_with_network_id)
+        server.resource_id = 'test_server'
         port_ids = [{'id': 1122}, {'id': 3344}]
         external_port_ids = [{'id': 5566}]
         server._data = {"internal_ports": jsonutils.dumps(port_ids),
                         "external_ports": jsonutils.dumps(external_port_ids)}
-        data_set = self.patchobject(server, 'data_set')
-
-        port1_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet1',
-                'ip_address': '41.41.41.41'
-            }
-        }
-        port2_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet2',
-                'ip_address': '42.42.42.42'
-            }
-        }
-        port3_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet3',
-                'ip_address': '43.43.43.43'
-            }
-        }
-        self.port_show.side_effect = [{'port': port1_fixed_ip},
-                                      {'port': port2_fixed_ip},
-                                      {'port': port3_fixed_ip}]
+        self.patchobject(nova.NovaClientPlugin, 'interface_detach')
+        self.patchobject(nova.NovaClientPlugin, 'check_interface_detach',
+                         return_value=True)
 
         server.prepare_for_replace()
 
-        # check, that data was updated
-        port_ids[0].update(port1_fixed_ip)
-        port_ids[1].update(port2_fixed_ip)
-        external_port_ids[0].update(port3_fixed_ip)
-
-        expected_data = jsonutils.dumps(port_ids)
-        expected_external_data = jsonutils.dumps(external_port_ids)
-        data_set.assert_has_calls([
-            mock.call('internal_ports', expected_data),
-            mock.call('external_ports', expected_external_data)])
-
-        # check, that all ip were removed from ports
-        empty_fixed_ips = {'port': {'fixed_ips': []}}
-        self.port_update.assert_has_calls([
-            mock.call(1122, empty_fixed_ips),
-            mock.call(3344, empty_fixed_ips),
-            mock.call(5566, empty_fixed_ips)])
+        # check, that the ports were detached from server
+        nova.NovaClientPlugin.interface_detach.assert_has_calls([
+            mock.call('test_server', 1122),
+            mock.call('test_server', 3344),
+            mock.call('test_server', 5566)])
 
     def test_restore_ports_after_rollback(self):
         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}]
         external_port_ids = [{'id': 5566}]
         server._data = {"internal_ports": jsonutils.dumps(port_ids),
                         "external_ports": jsonutils.dumps(external_port_ids)}
-        port1_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet1',
-                'ip_address': '41.41.41.41'
-            }
-        }
-        port2_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet2',
-                'ip_address': '42.42.42.42'
-            }
-        }
-        port3_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet3',
-                'ip_address': '43.43.43.43'
-            }
-        }
-        port_ids[0].update(port1_fixed_ip)
-        port_ids[1].update(port2_fixed_ip)
-        external_port_ids[0].update(port3_fixed_ip)
+
         # add data to old server in backup stack
         old_server = mock.Mock()
+        old_server.resource_id = 'old_server'
         stack._backup_stack = mock.Mock()
         stack._backup_stack().resources.get.return_value = old_server
         old_server._data_get_ports.side_effect = [port_ids, external_port_ids]
 
+        self.patchobject(nova.NovaClientPlugin, 'interface_detach')
+        self.patchobject(nova.NovaClientPlugin, 'check_interface_detach',
+                         return_value=True)
+        self.patchobject(nova.NovaClientPlugin, 'interface_attach')
+        self.patchobject(nova.NovaClientPlugin, 'check_interface_attach',
+                         return_value=True)
+
         server.restore_prev_rsrc()
 
-        # check, that all ip were removed from new_ports
-        empty_fixed_ips = {'port': {'fixed_ips': []}}
-        self.port_update.assert_has_calls([
-            mock.call(1122, empty_fixed_ips),
-            mock.call(3344, empty_fixed_ips),
-            mock.call(5566, empty_fixed_ips)])
+        # check, that ports were detached from new server
+        nova.NovaClientPlugin.interface_detach.assert_has_calls([
+            mock.call('existing_server', 1122),
+            mock.call('existing_server', 3344),
+            mock.call('existing_server', 5566)])
 
-        # check, that all ip were restored for old_ports
-        self.port_update.assert_has_calls([
-            mock.call(1122, {'port': port1_fixed_ip}),
-            mock.call(3344, {'port': port2_fixed_ip}),
-            mock.call(5566, {'port': port3_fixed_ip})])
+        # check, that ports were attached to old server
+        nova.NovaClientPlugin.interface_attach.assert_has_calls([
+            mock.call('old_server', 1122),
+            mock.call('old_server', 3344),
+            mock.call('old_server', 5566)])
+
+    def test_restore_ports_after_rollback_attach_failed(self):
+        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)}
+
+        # add data to old server in backup stack
+        old_server = mock.Mock()
+        old_server.resource_id = 'old_server'
+        stack._backup_stack = mock.Mock()
+        stack._backup_stack().resources.get.return_value = old_server
+        old_server._data_get_ports.side_effect = [port_ids, []]
+
+        class Fake(object):
+            def interface_list(self):
+                return [iface(1122)]
+        iface = collections.namedtuple('iface', ['port_id'])
+
+        self.patchobject(nova.NovaClientPlugin, 'interface_detach')
+        self.patchobject(nova.NovaClientPlugin, 'check_interface_detach',
+                         return_value=True)
+        self.patchobject(nova.NovaClientPlugin, 'interface_attach')
+        self.patchobject(nova.NovaClientPlugin, 'fetch_server')
+        # need to mock 11 times: 1 for port 1122, 10 for port 3344
+        nova.NovaClientPlugin.fetch_server.side_effect = [Fake()] * 11
+
+        exc = self.assertRaises(exception.InterfaceAttachFailed,
+                                server.restore_prev_rsrc)
+        self.assertIn('Failed to attach interface (3344) to server '
+                      '(old_server)',
+                      six.text_type(exc))
 
     def test_restore_ports_after_rollback_convergence(self):
         t = template_format.parse(tmpl_server_with_network_id)
@@ -4929,14 +4940,17 @@ class ServerInternalPortTest(common.HeatTestCase):
 
         # mock resource from previous template
         prev_rsrc = stack['server']
-        prev_rsrc.resource_id = 'prev-rsrc'
         # store in db
         prev_rsrc.state_set(prev_rsrc.UPDATE, prev_rsrc.COMPLETE)
+        prev_rsrc.resource_id = 'prev_rsrc'
 
         # mock resource from existing template, store in db, and set _data
-        existing_rsrc = stack['server']
+        resource_defns = stack.t.resource_definitions(stack)
+        existing_rsrc = servers.Server('server', resource_defns['server'],
+                                       stack)
+        existing_rsrc.stack = stack
         existing_rsrc.current_template_id = stack.t.id
-        existing_rsrc.resource_id = 'existing-rsrc'
+        existing_rsrc.resource_id = 'existing_rsrc'
         existing_rsrc.state_set(existing_rsrc.UPDATE, existing_rsrc.COMPLETE)
 
         port_ids = [{'id': 1122}, {'id': 3344}]
@@ -4948,47 +4962,26 @@ class ServerInternalPortTest(common.HeatTestCase):
         # mock previous resource was replaced by existing resource
         prev_rsrc.replaced_by = existing_rsrc.id
 
-        port1_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet1',
-                'ip_address': '41.41.41.41'
-            }
-        }
-        port2_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet2',
-                'ip_address': '42.42.42.42'
-            }
-        }
-        port3_fixed_ip = {
-            'fixed_ips': {
-                'subnet_id': 'test_subnet3',
-                'ip_address': '43.43.43.43'
-            }
-        }
-        port_ids[0].update(port1_fixed_ip)
-        port_ids[1].update(port2_fixed_ip)
-        external_port_ids[0].update(port3_fixed_ip)
-        # add data to old server
-        prev_rsrc._data = {
-            "internal_ports": jsonutils.dumps(port_ids),
-            "external_ports": jsonutils.dumps(external_port_ids)
-        }
+        self.patchobject(nova.NovaClientPlugin, 'interface_detach')
+        self.patchobject(nova.NovaClientPlugin, 'check_interface_detach',
+                         return_value=True)
+        self.patchobject(nova.NovaClientPlugin, 'interface_attach')
+        self.patchobject(nova.NovaClientPlugin, 'check_interface_attach',
+                         return_value=True)
 
         prev_rsrc.restore_prev_rsrc(convergence=True)
 
-        # check, that all ip were removed from new_ports
-        empty_fixed_ips = {'port': {'fixed_ips': []}}
-        self.port_update.assert_has_calls([
-            mock.call(1122, empty_fixed_ips),
-            mock.call(3344, empty_fixed_ips),
-            mock.call(5566, empty_fixed_ips)])
+        # check, that ports were detached from existing server
+        nova.NovaClientPlugin.interface_detach.assert_has_calls([
+            mock.call('existing_rsrc', 1122),
+            mock.call('existing_rsrc', 3344),
+            mock.call('existing_rsrc', 5566)])
 
-        # check, that all ip were restored for old_ports
-        self.port_update.assert_has_calls([
-            mock.call(1122, {'port': port1_fixed_ip}),
-            mock.call(3344, {'port': port2_fixed_ip}),
-            mock.call(5566, {'port': port3_fixed_ip})])
+        # check, that ports were attached to old server
+        nova.NovaClientPlugin.interface_attach.assert_has_calls([
+            mock.call('prev_rsrc', 1122),
+            mock.call('prev_rsrc', 3344),
+            mock.call('prev_rsrc', 5566)])
 
     def test_store_external_ports_os_interface_not_installed(self):
         t, stack, server = self._return_template_stack_and_rsrc_defn(