From 938262073f11913169c79299941d9c5c4d4b0645 Mon Sep 17 00:00:00 2001 From: Shubham Potale Date: Thu, 21 Nov 2019 20:40:56 +0530 Subject: [PATCH] Delete VNF should fail with 409 error If user deletes VNF in "PENDING_CREATE" status, VNF is deleted from tacker but the resources created by VIM driver are not cleaned up properly. This patch doesn't allow VNF to be deleted if the status is in "PENDING_CREATE" status. It will raise 409 error. APIImpact Return 409 error instead of 200 when VNF is deleted in "PENDING_CREATE" status. Change-Id: I2de9c3dce5a4b3795119e2550aa8acea68940c45 Closes-Bug: #1798726 --- api-ref/source/v1/parameters.yaml | 6 ++++ .../v1/samples/vnfs/vnfs-delete-request.json | 8 +++++ api-ref/source/v1/vnfs.inc | 14 ++++++++ tacker/db/vnfm/vnfm_db.py | 31 ++++++++++++------ tacker/tests/unit/vnfm/test_plugin.py | 32 +++++++++++++------ tacker/vnfm/plugin.py | 23 +++++++------ 6 files changed, 86 insertions(+), 28 deletions(-) create mode 100644 api-ref/source/v1/samples/vnfs/vnfs-delete-request.json diff --git a/api-ref/source/v1/parameters.yaml b/api-ref/source/v1/parameters.yaml index 316d52301..2b1305f5e 100644 --- a/api-ref/source/v1/parameters.yaml +++ b/api-ref/source/v1/parameters.yaml @@ -731,6 +731,12 @@ vnf_error_reason: in: body required: true type: string +vnf_force_delete_flag: + description: | + VNF attributes object with ``force`` key which can contain true/false value. + in: body + required: false + type: object vnf_heat_template: description: | Heat template which is translated from the VNFD template. diff --git a/api-ref/source/v1/samples/vnfs/vnfs-delete-request.json b/api-ref/source/v1/samples/vnfs/vnfs-delete-request.json new file mode 100644 index 000000000..00284f7ea --- /dev/null +++ b/api-ref/source/v1/samples/vnfs/vnfs-delete-request.json @@ -0,0 +1,8 @@ +{ + "vnf": { + "attributes": { + "force": true + } + } +} + diff --git a/api-ref/source/v1/vnfs.inc b/api-ref/source/v1/vnfs.inc index 01ea07166..150988df9 100644 --- a/api-ref/source/v1/vnfs.inc +++ b/api-ref/source/v1/vnfs.inc @@ -266,6 +266,11 @@ Delete VNF Deletes a given VNF. +.. note:: + Non-admin users are not allowed to delete a VNF in ``PENDING_CREATE`` and + ``PENDING_DELETE`` status. In this case, it would return 409 error. Admin + users can delete a VNF in any status with force flag true. + Response Codes -------------- @@ -286,6 +291,15 @@ Request Parameters .. rest_parameters:: parameters.yaml - vnf_id: vnf_id_path + - attributes: vnf_force_delete_flag + +Request Example +--------------- + +Admin user can request to delete VNF forcefully. + +.. literalinclude:: samples/vnfs/vnfs-delete-request.json + :language: javascript List VNF resources ================== diff --git a/tacker/db/vnfm/vnfm_db.py b/tacker/db/vnfm/vnfm_db.py index 6c95fbfdb..45e2a3ba3 100644 --- a/tacker/db/vnfm/vnfm_db.py +++ b/tacker/db/vnfm/vnfm_db.py @@ -492,8 +492,9 @@ class VNFMPluginDb(vnfm.VNFMPluginBase, db_base.CommonDbMixin): return vnf_db def _update_vnf_status_db(self, context, vnf_id, current_statuses, - new_status): - vnf_db = self._get_vnf_db(context, vnf_id, current_statuses) + new_status, vnf_db=None): + if not vnf_db: + vnf_db = self._get_vnf_db(context, vnf_id, current_statuses) if self.check_vnf_status_legality(vnf_db, vnf_id): vnf_db.update({'status': new_status}) return vnf_db @@ -508,7 +509,7 @@ class VNFMPluginDb(vnfm.VNFMPluginBase, db_base.CommonDbMixin): def check_vnf_status_legality(vnf_db, vnf_id): if vnf_db.status == constants.PENDING_DELETE: error_reason = _("Operation on PENDING_DELETE VNF " - "is not permited. Please contact your " + "is not permitted. Please contact your " "Administrator.") raise vnfm.VNFDeleteFailed(reason=error_reason) if(vnf_db.status in [constants.PENDING_UPDATE, @@ -585,13 +586,25 @@ class VNFMPluginDb(vnfm.VNFMPluginBase, db_base.CommonDbMixin): ns_db.NS.vnf_ids.like("%" + vnf_id + "%")).first() if not force_delete: - if (nss_db is not None and nss_db.status not in - [constants.PENDING_DELETE, constants.ERROR]): - raise vnfm.VNFInUse(vnf_id=vnf_id) + # If vnf is deleted by NFVO, then vnf_id would + # exist in the nss_db otherwise it should be queried from + # vnf db table. + if nss_db is not None: + if nss_db.status not in [constants.PENDING_DELETE, + constants.ERROR]: + raise vnfm.VNFInUse(vnf_id=vnf_id) + else: + vnf_db = self._get_vnf_db(context, vnf_id, + _ACTIVE_UPDATE_ERROR_DEAD) + if (vnf_db is not None and vnf_db.status == constants. + PENDING_CREATE): + raise vnfm.VNFInUse( + message="Operation on PENDING_CREATE VNF is not " + "permitted.") - vnf_db = self._update_vnf_status_db( - context, vnf_id, _ACTIVE_UPDATE_ERROR_DEAD, - constants.PENDING_DELETE) + vnf_db = self._update_vnf_status_db( + context, vnf_id, _ACTIVE_UPDATE_ERROR_DEAD, + constants.PENDING_DELETE, vnf_db=vnf_db) else: vnf_db = self._update_vnf_status_db_no_check(context, vnf_id, _ACTIVE_UPDATE_ERROR_DEAD, diff --git a/tacker/tests/unit/vnfm/test_plugin.py b/tacker/tests/unit/vnfm/test_plugin.py index 2b8eca03e..6895fb9c7 100644 --- a/tacker/tests/unit/vnfm/test_plugin.py +++ b/tacker/tests/unit/vnfm/test_plugin.py @@ -15,6 +15,7 @@ from datetime import datetime +import ddt import mock from mock import patch from oslo_utils import uuidutils @@ -131,6 +132,7 @@ class TestVNFMPluginMonitor(db_base.SqlTestCase): self.assertEqual(1, len(hosting_vnfs)) +@ddt.ddt class TestVNFMPlugin(db_base.SqlTestCase): def setUp(self): super(TestVNFMPlugin, self).setUp() @@ -254,7 +256,7 @@ class TestVNFMPlugin(db_base.SqlTestCase): session.flush() return vnfd_attr - def _insert_dummy_vnf(self): + def _insert_dummy_vnf(self, status="ACTIVE"): session = self.context.session vnf_db = vnfm_db.VNF( id='6261579e-d6f3-49ad-8bc3-a9cb974778fe', @@ -265,13 +267,13 @@ class TestVNFMPlugin(db_base.SqlTestCase): vnfd_id='eb094833-995e-49f0-a047-dfb56aaf7c4e', vim_id='6261579e-d6f3-49ad-8bc3-a9cb974778ff', placement_attr={'region': 'RegionOne'}, - status='ACTIVE', + status=status, deleted_at=datetime.min) session.add(vnf_db) session.flush() return vnf_db - def _insert_dummy_pending_vnf(self, context): + def _insert_dummy_pending_vnf(self, context, status='PENDING_DELETE'): session = context.session vnf_db = vnfm_db.VNF( id='6261579e-d6f3-49ad-8bc3-a9cb974778fe', @@ -282,7 +284,7 @@ class TestVNFMPlugin(db_base.SqlTestCase): vnfd_id='eb094833-995e-49f0-a047-dfb56aaf7c4e', vim_id='6261579e-d6f3-49ad-8bc3-a9cb974778ff', placement_attr={'region': 'RegionOne'}, - status='PENDING_DELETE', + status=status, deleted_at=datetime.min) session.add(vnf_db) session.flush() @@ -510,7 +512,7 @@ class TestVNFMPlugin(db_base.SqlTestCase): self.vnfm_plugin.create_vnf, self.context, vnf_obj) - @patch('tacker.vnfm.plugin.VNFMPlugin.delete_vnf') + @patch('tacker.vnfm.plugin.VNFMPlugin._delete_vnf') def test_create_vnf_fail(self, mock_delete_vnf): self._insert_dummy_vnf_template() vnf_obj = utils.get_dummy_vnf_obj() @@ -518,8 +520,9 @@ class TestVNFMPlugin(db_base.SqlTestCase): self.assertRaises(vnfm.HeatClientException, self.vnfm_plugin.create_vnf, self.context, vnf_obj) - vnf_id = self.vnfm_plugin.delete_vnf.call_args[0][1] - mock_delete_vnf.assert_called_once_with(self.context, vnf_id) + vnf_id = self.vnfm_plugin._delete_vnf.call_args[0][1] + mock_delete_vnf.assert_called_once_with(self.context, vnf_id, + force_delete=True) def test_create_vnf_create_wait_failed_exception(self): self._insert_dummy_vnf_template() @@ -610,9 +613,10 @@ class TestVNFMPlugin(db_base.SqlTestCase): self.context, dummy_vnf_obj['id']) - def test_force_delete_vnf(self): + @ddt.data('PENDING_DELETE', 'PENDING_CREATE') + def test_force_delete_vnf(self, status): self._insert_dummy_vnf_template() - dummy_vnf_obj = self._insert_dummy_pending_vnf(self.context) + dummy_vnf_obj = self._insert_dummy_pending_vnf(self.context, status) vnfattr = {'vnf': {'attributes': {'force': True}}} self.vnfm_plugin.delete_vnf(self.context, dummy_vnf_obj[ 'id'], vnf=vnfattr) @@ -624,7 +628,7 @@ class TestVNFMPlugin(db_base.SqlTestCase): self._cos_db_plugin.create_event.assert_called_with( self.context, evt_type=constants.RES_EVT_DELETE, res_id=mock.ANY, res_state=mock.ANY, res_type=constants.RES_TYPE_VNF, - tstamp=mock.ANY, details=mock.ANY) + tstamp=mock.ANY, details="VNF Delete Complete") def test_force_delete_vnf_non_admin(self): self._insert_dummy_vnf_template() @@ -667,6 +671,14 @@ class TestVNFMPlugin(db_base.SqlTestCase): mock.ANY, mock.ANY) + def test_delete_vnf_failed_with_status_pending_create(self): + self._insert_dummy_vnf_template() + dummy_device_obj_with_pending_create_status = self. \ + _insert_dummy_vnf(status="PENDING_CREATE") + self.assertRaises(vnfm.VNFInUse, self.vnfm_plugin.delete_vnf, + self.context, + dummy_device_obj_with_pending_create_status['id']) + def _insert_dummy_ns_template(self): session = self.context.session attributes = { diff --git a/tacker/vnfm/plugin.py b/tacker/vnfm/plugin.py index 827066e37..17b57dc0e 100644 --- a/tacker/vnfm/plugin.py +++ b/tacker/vnfm/plugin.py @@ -376,7 +376,7 @@ class VNFMPlugin(vnfm_db.VNFMPluginDb, VNFMMgmtMixin): 'so delete this vnf', vnf_dict['id']) with excutils.save_and_reraise_exception(): - self.delete_vnf(context, vnf_id) + self._delete_vnf(context, vnf_id, force_delete=True) vnf_dict['instance_id'] = instance_id return vnf_dict @@ -621,15 +621,8 @@ class VNFMPlugin(vnfm_db.VNFMPluginDb, VNFMMgmtMixin): self.mgmt_delete_post(context, vnf_dict) self._delete_vnf_post(context, vnf_dict, e) - def delete_vnf(self, context, vnf_id, vnf=None): + def _delete_vnf(self, context, vnf_id, force_delete=False): - # Extract "force_delete" from request's body - force_delete = False - if vnf and vnf['vnf'].get('attributes').get('force'): - force_delete = vnf['vnf'].get('attributes').get('force') - if force_delete and not context.is_admin: - LOG.warning("force delete is admin only operation") - raise exceptions.AdminRequired(reason="Admin only operation") vnf_dict = self._delete_vnf_pre(context, vnf_id, force_delete=force_delete) driver_name, vim_auth = self._get_infra_driver(context, vnf_dict) @@ -673,6 +666,18 @@ class VNFMPlugin(vnfm_db.VNFMPluginDb, VNFMMgmtMixin): self.spawn_n(self._delete_vnf_wait, context, vnf_dict, vim_auth, driver_name) + def delete_vnf(self, context, vnf_id, vnf=None): + + # Extract "force_delete" from request's body + force_delete = False + if vnf and vnf['vnf'].get('attributes').get('force'): + force_delete = vnf['vnf'].get('attributes').get('force') + if force_delete and not context.is_admin: + LOG.warning("force delete is admin only operation") + raise exceptions.AdminRequired(reason="Admin only operation") + + self._delete_vnf(context, vnf_id, force_delete=force_delete) + def _handle_vnf_scaling(self, context, policy): # validate def _validate_scaling_policy():