openstack/ironic
Code Issues Proposed changes

Merge "Remove vifs upon teardown"

Browse Source
This commit is contained in:
Zuul 2018-04-17 04:19:38 +00:00 committed by Gerrit Code Review
commit 084c9161f1
7 changed files with 129 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
ironic/common/network.py
View File

@ -12,8 +12,12 @@
# License for the specific language governing permissions and limitations
# under the License.
from oslo_log import log
from ironic.common import exception
LOG = log.getLogger(__name__)
def get_node_vif_ids(task):
"""Get all VIF ids for a node.
@ -108,3 +112,34 @@ def get_physnets_by_portgroup_id(task, portgroup_id, exclude_port=None):
raise exception.PortgroupPhysnetInconsistent(
portgroup=portgroup.uuid, physical_networks=", ".join(pg_physnets))
return pg_physnets
def remove_vifs_from_node(task):
"""Remove all vif attachment records from a node.
:param task: a TaskManager instance.
"""
vifs = task.driver.network.vif_list(task)
for vif_entry in vifs:
vif = vif_entry.get('id')
if not vif:
LOG.warning('Incorrect vif entry for %(node)s lacks an ID field, '
'and is thus unsupported. Found: %(found)s.',
{'node': task.node.uuid,
'found': vif_entry})
continue
try:
task.driver.network.vif_detach(task, vif)
except exception.VifNotAttached as e:
LOG.warning('While removing records of VIF attachments from node '
'%(node)s, we recieved indication that %(vif)s is '
'no longer attached. There should not happen under '
'normal circumstances.',
{'node': task.node.uuid,
'vif': vif})
except exception.NetworkError as e:
LOG.error('An error has been encountered while removing a '
'VIF record for %(node)s. Error: %(error)s',
{'node': task.node.uuid,
'error': e})

2
ironic/conductor/manager.py
View File

@ -62,6 +62,7 @@ from ironic.common import exception
from ironic.common.glance_service import service_utils as glance_utils
from ironic.common.i18n import _
from ironic.common import images
from ironic.common import network
from ironic.common import states
from ironic.common import swift
from ironic.conductor import base_manager
@ -929,6 +930,7 @@ class ConductorManager(base_manager.BaseConductorManager):
driver_internal_info = node.driver_internal_info
driver_internal_info.pop('instance', None)
node.driver_internal_info = driver_internal_info
network.remove_vifs_from_node(task)
node.save()
# Begin cleaning

6
ironic/drivers/modules/network/common.py
View File

@ -570,6 +570,8 @@ class NeutronVIFPortIDMixin(VIFPortIDMixin):
self._clear_vif_from_port_like_obj(port_like_obj)
# NOTE(vsaienko) allow to unplug VIFs from ACTIVE instance.
if task.node.provision_state == states.ACTIVE:
# NOTE(vsaienko): allow to unplug VIFs from ACTIVE instance.
# NOTE(TheJulia): Also ensure that we delete the vif when in
# DELETING state.
if task.node.provision_state in [states.ACTIVE, states.DELETING]:
neutron.unbind_neutron_port(vif_id, context=task.context)

45
ironic/tests/unit/common/test_network.py
View File

@ -13,11 +13,15 @@
# License for the specific language governing permissions and limitations
# under the License.
import mock
from oslo_utils import uuidutils
from ironic.common import exception
from ironic.common import network
from ironic.common import neutron as neutron_common
from ironic.common import states
from ironic.conductor import task_manager
from ironic.drivers.modules.network import common as driver_common
from ironic.tests.unit.conductor import mgr_utils
from ironic.tests.unit.db import base as db_base
from ironic.tests.unit.db import utils as db_utils
@ -156,6 +160,47 @@ class TestNetwork(db_base.DbTestCase):
def test_get_node_vif_ids_during_rescuing(self):
self._test_get_node_vif_ids_multitenancy('rescuing_vif_port_id')
def test_remove_vifs_from_node(self):
db_utils.create_test_port(
node_id=self.node.id, address='aa:bb:cc:dd:ee:ff',
internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-A'})
db_utils.create_test_portgroup(
node_id=self.node.id, address='dd:ee:ff:aa:bb:cc',
internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-B'})
with task_manager.acquire(self.context, self.node.uuid) as task:
network.remove_vifs_from_node(task)
with task_manager.acquire(self.context, self.node.uuid) as task:
result = network.get_node_vif_ids(task)
self.assertEqual({}, result['ports'])
self.assertEqual({}, result['portgroups'])
class TestRemoveVifsTestCase(db_base.DbTestCase):
def setUp(self):
super(TestRemoveVifsTestCase, self).setUp()
self.node = object_utils.create_test_node(
self.context,
network_interface='flat',
provision_state=states.DELETING)
@mock.patch.object(neutron_common, 'unbind_neutron_port')
def test_remove_vifs_from_node_failure(self, mock_unbind):
db_utils.create_test_port(
node_id=self.node.id, address='aa:bb:cc:dd:ee:ff',
internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-A'})
db_utils.create_test_portgroup(
node_id=self.node.id, address='dd:ee:ff:aa:bb:cc',
internal_info={driver_common.TENANT_VIF_KEY: 'test-vif-B'})
mock_unbind.side_effect = [exception.NetworkError, None]
with task_manager.acquire(self.context, self.node.uuid) as task:
network.remove_vifs_from_node(task)
with task_manager.acquire(self.context, self.node.uuid) as task:
result = network.get_node_vif_ids(task)
self.assertEqual({}, result['ports'])
self.assertEqual({}, result['portgroups'])
self.assertEqual(2, mock_unbind.call_count)
class GetPortgroupByIdTestCase(db_base.DbTestCase):

12
ironic/tests/unit/conductor/test_manager.py
View File

@ -1791,9 +1791,13 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
self.assertEqual({}, node.instance_info)
mock_tear_down.assert_called_once_with(mock.ANY)
# TODO(TheJulia): Since we're functionally bound to neutron support
# by default, the fake drivers still invoke neutron.
@mock.patch('ironic.common.neutron.unbind_neutron_port')
@mock.patch('ironic.conductor.manager.ConductorManager._do_node_clean')
@mock.patch('ironic.drivers.modules.fake.FakeDeploy.tear_down')
def test__do_node_tear_down_ok(self, mock_tear_down, mock_clean):
def test__do_node_tear_down_ok(self, mock_tear_down, mock_clean,
mock_unbind):
# test when driver.deploy.tear_down succeeds
node = obj_utils.create_test_node(
self.context, driver='fake', provision_state=states.DELETING,
@ -1802,11 +1806,15 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
instance_info={'foo': 'bar'},
driver_internal_info={'is_whole_disk_image': False,
'instance': {'ephemeral_gb': 10}})
port = obj_utils.create_test_port(
self.context, node_id=node.id,
internal_info={'tenant_vif_port_id': 'foo'})
task = task_manager.TaskManager(self.context, node.uuid)
self._start_service()
self.service._do_node_tear_down(task, node.provision_state)
node.refresh()
port.refresh()
# Node will be moved to AVAILABLE after cleaning, not tested here
self.assertEqual(states.CLEANING, node.provision_state)
self.assertEqual(states.AVAILABLE, node.target_provision_state)
@ -1816,6 +1824,8 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin,
self.assertNotIn('instance', node.driver_internal_info)
mock_tear_down.assert_called_once_with(mock.ANY)
mock_clean.assert_called_once_with(mock.ANY)
self.assertEqual({}, port.internal_info)
mock_unbind.assert_called_once_with('foo', context=mock.ANY)
@mock.patch('ironic.drivers.modules.fake.FakeRescue.clean_up')
@mock.patch('ironic.conductor.manager.ConductorManager._do_node_clean')

14
ironic/tests/unit/drivers/modules/network/test_common.py
View File

@ -931,6 +931,20 @@ class TestNeutronVifPortIDMixin(db_base.DbTestCase):
mock_get.assert_called_once_with(task, 'fake_vif_id')
mock_clear.assert_called_once_with(self.port)
@mock.patch.object(common.VIFPortIDMixin, '_clear_vif_from_port_like_obj')
@mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True)
@mock.patch.object(common.VIFPortIDMixin, '_get_port_like_obj_by_vif_id')
def test_vif_detach_deleting_node(self, mock_get, mock_unp, mock_clear):
self.node.provision_state = states.DELETING
self.node.save()
mock_get.return_value = self.port
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.vif_detach(task, 'fake_vif_id')
mock_unp.assert_called_once_with('fake_vif_id',
context=task.context)
mock_get.assert_called_once_with(task, 'fake_vif_id')
mock_clear.assert_called_once_with(self.port)
@mock.patch.object(common.VIFPortIDMixin, '_clear_vif_from_port_like_obj')
@mock.patch.object(neutron_common, 'unbind_neutron_port', autospec=True)
@mock.patch.object(common.VIFPortIDMixin, '_get_port_like_obj_by_vif_id')

18
releasenotes/notes/remove-vifs-on-teardown-707c8e40c46b6e64.yaml Normal file
View File

@ -0,0 +1,18 @@
---
upgrade:
- |
The behavior for retention of VIF interface attachments has changed.
If your use of the BareMetal service is reliant upon the behavior of
the VIFs being retained, which was introduced as a behavior change
during the Ocata cycle, then you must update your tooling to explicitly
re-add the the VIF attachments prior to deployment.
fixes:
- |
Removes all records of VIF attachments upon the teardown of a deployed
node. This is in order to resolve issues related to where it is
operationally impossible in some circumstances to remove a VIF
attachment while a node is being undeployed as the Compute service
will only attempt to remove the VIF for five minutes.
See `bug 1743652 <https://bugs.launchpad.net/ironic/+bug/1743652>`_ for more details.