From 45ac7ec7b924197096e5ee3efff4b2c5139e5bfa Mon Sep 17 00:00:00 2001 From: Hiromu Asahina Date: Fri, 4 Feb 2022 20:20:59 +0900 Subject: [PATCH] Refactor v1 vnflcm_driver The following points in the vnflcm_driver reduce the readability and might be potential bugs. (i) There are some unused variables, e.g., ``scale_id_list`` in ``_scale_vnf_pre``. (ii) ``else`` in ``_scale_vnf_pre`` looks inappropriate because all unknown request types are considered as 'SCALE_OUT'. (iii) ``_scale_vnf`` is redundant. It can be merged into `scale`. This patch solves these problems as follows. (i) Removed unused variables. (ii) Wrote ``elif scale_vnf_request.type == 'SCALE_OUT':`` explicitly. (iii) Merged ``_scale_vnf`` and ``scale`` and renamed ``scale`` to ``_scale_vnf`` as most LCM operation methods consist of a pair of a protected method and public method, e.g., `heal_vnf and _heal_vnf`. Change-Id: I49457aa35458a68f2294c229b440ba1c68cbfe91 --- .../tests/unit/vnflcm/test_vnflcm_driver.py | 92 +++++++++++++------ tacker/vnflcm/vnflcm_driver.py | 54 +++++------ 2 files changed, 87 insertions(+), 59 deletions(-) diff --git a/tacker/tests/unit/vnflcm/test_vnflcm_driver.py b/tacker/tests/unit/vnflcm/test_vnflcm_driver.py index e7306b61c..c6ab37d98 100644 --- a/tacker/tests/unit/vnflcm/test_vnflcm_driver.py +++ b/tacker/tests/unit/vnflcm/test_vnflcm_driver.py @@ -1797,6 +1797,7 @@ class TestVnflcmDriver(db_base.SqlTestCase): @mock.patch.object(driver_manager.DriverManager, "invoke") def test_scale_true(self, mock_invoke, mock_init_hash, mock_get_service_plugins): + op_occ = mock.MagicMock() mock_init_hash.return_value = { "vnflcm_noop": "ffea638bfdbde3fb01f191bbe75b031859" "b18d663b127100eb72b19eecd7ed51" @@ -1810,11 +1811,18 @@ class TestVnflcmDriver(db_base.SqlTestCase): vnf_info['vnf_lcm_op_occ'] = mock.ANY vim_connection_info = vim_connection.VimConnectionInfo( vim_type="openstack") + update = {'vim_connection_info': [vim_connection_info]} + scale_status = objects.ScaleInfo(aspect_id='vdu1_aspect', + scale_level=1) + vnf_instance = fakes.return_vnf_instance( + fields.VnfInstanceState.INSTANTIATED, scale_status=scale_status, + **update) scale_name_list = ["fake"] grp_id = "fake_id" driver = vnflcm_driver.VnfLcmDriver() - driver.scale(self.context, vnf_info, scale_vnf_request, - vim_connection_info, scale_name_list, grp_id) + driver._scale_vnf(self.context, vnf_info, vnf_instance, + scale_vnf_request, vim_connection_info, + scale_name_list, grp_id, op_occ) @mock.patch.object(TackerManager, 'get_service_plugins', return_value={'VNFM': FakeVNFMPlugin()}) @@ -1825,6 +1833,7 @@ class TestVnflcmDriver(db_base.SqlTestCase): def test_scale_false_in(self, mock_invoke, mock_safe_load, mock_init_hash, mock_get_service_plugins): + op_occ = mock.MagicMock() mock_init_hash.return_value = { "vnflcm_noop": "ffea638bfdbde3fb01f191bbe75b031859" "b18d663b127100eb72b19eecd7ed51" @@ -1838,14 +1847,22 @@ class TestVnflcmDriver(db_base.SqlTestCase): vnf_info['vnf_lcm_op_occ'] = mock.ANY vim_connection_info = vim_connection.VimConnectionInfo( vim_type="openstack") + update = {'vim_connection_info': [vim_connection_info]} + scale_status = objects.ScaleInfo(aspect_id='vdu1_aspect', + scale_level=1) + vnf_instance = fakes.return_vnf_instance( + fields.VnfInstanceState.INSTANTIATED, scale_status=scale_status, + **update) scale_name_list = ["fake"] grp_id = "fake_id" with open(vnf_info["attributes"]["heat_template"], "r") as f: mock_safe_load.return_value = yaml.safe_load(f) print(mock_safe_load.return_value) driver = vnflcm_driver.VnfLcmDriver() - driver.scale(self.context, vnf_info, scale_vnf_request, - vim_connection_info, scale_name_list, grp_id) + + driver._scale_vnf(self.context, vnf_info, vnf_instance, + scale_vnf_request, vim_connection_info, + scale_name_list, grp_id, op_occ) @mock.patch.object(TackerManager, 'get_service_plugins', return_value={'VNFM': FakeVNFMPlugin()}) @@ -1856,6 +1873,7 @@ class TestVnflcmDriver(db_base.SqlTestCase): def test_scale_false_out_initial(self, mock_invoke, mock_safe_load, mock_init_hash, mock_get_service_plugins): + op_occ = mock.MagicMock() mock_init_hash.return_value = { "vnflcm_noop": "ffea638bfdbde3fb01f191bbe75b031859" "b18d663b127100eb72b19eecd7ed51" @@ -1869,14 +1887,22 @@ class TestVnflcmDriver(db_base.SqlTestCase): vnf_info['vnf_lcm_op_occ'] = mock.ANY vim_connection_info = vim_connection.VimConnectionInfo( vim_type="openstack") + update = {'vim_connection_info': [vim_connection_info]} + scale_status = objects.ScaleInfo(aspect_id='vdu1_aspect', + scale_level=1) + vnf_instance = fakes.return_vnf_instance( + fields.VnfInstanceState.INSTANTIATED, scale_status=scale_status, + **update) scale_name_list = ["fake"] grp_id = "fake_id" with open(vnf_info["attributes"]["heat_template"], "r") as f: mock_safe_load.return_value = yaml.safe_load(f) print(mock_safe_load.return_value) driver = vnflcm_driver.VnfLcmDriver() - driver.scale(self.context, vnf_info, scale_vnf_request, - vim_connection_info, scale_name_list, grp_id) + + driver._scale_vnf(self.context, vnf_info, vnf_instance, + scale_vnf_request, vim_connection_info, + scale_name_list, grp_id, op_occ) @mock.patch.object(TackerManager, 'get_service_plugins', return_value={'VNFM': FakeVNFMPlugin()}) @@ -1887,6 +1913,7 @@ class TestVnflcmDriver(db_base.SqlTestCase): def test_scale_false_out_level_up(self, mock_invoke, mock_safe_load, mock_init_hash, mock_get_service_plugins): + op_occ = mock.MagicMock() mock_init_hash.return_value = { "vnflcm_noop": "ffea638bfdbde3fb01f191bbe75b031859" "b18d663b127100eb72b19eecd7ed51" @@ -1900,14 +1927,21 @@ class TestVnflcmDriver(db_base.SqlTestCase): vnf_info['vnf_lcm_op_occ'] = mock.ANY vim_connection_info = vim_connection.VimConnectionInfo( vim_type="openstack") + update = {'vim_connection_info': [vim_connection_info]} + scale_status = objects.ScaleInfo( + aspect_id='vdu1_aspect', scale_level=1) + vnf_instance = fakes.return_vnf_instance( + fields.VnfInstanceState.INSTANTIATED, scale_status=scale_status, + **update) scale_name_list = ["fake"] grp_id = "fake_id" with open(vnf_info["attributes"]["heat_template"], "r") as f: mock_safe_load.return_value = yaml.safe_load(f) print(mock_safe_load.return_value) driver = vnflcm_driver.VnfLcmDriver() - driver.scale(self.context, vnf_info, scale_vnf_request, - vim_connection_info, scale_name_list, grp_id) + driver._scale_vnf(self.context, vnf_info, vnf_instance, + scale_vnf_request, vim_connection_info, + scale_name_list, grp_id, op_occ) @mock.patch.object(TackerManager, 'get_service_plugins', return_value={'VNFM': FakeVNFMPlugin()}) @@ -2009,6 +2043,7 @@ class TestVnflcmDriver(db_base.SqlTestCase): @mock.patch.object(driver_manager.DriverManager, "invoke") def test_scale_scale_type_unknown(self, mock_invoke, mock_init_hash, mock_get_service_plugins): + op_occ = mock.MagicMock() mock_init_hash.return_value = { "vnflcm_noop": "ffea638bfdbde3fb01f191bbe75b031859" "b18d663b127100eb72b19eecd7ed51" @@ -2021,20 +2056,22 @@ class TestVnflcmDriver(db_base.SqlTestCase): scale_vnf_request = fakes.scale_request("UNKNOWN", "SP1", 1, "True") vim_connection_info = vim_connection.VimConnectionInfo( vim_type="openstack") + update = {'vim_connection_info': [vim_connection_info]} + scale_status = objects.ScaleInfo( + aspect_id='vdu1_aspect', scale_level=1) + vnf_instance = fakes.return_vnf_instance( + fields.VnfInstanceState.INSTANTIATED, scale_status=scale_status, + **update) scale_name_list = ["fake"] grp_id = "fake_id" driver = vnflcm_driver.VnfLcmDriver() msg = 'Unknown scale type' - self.assertRaisesRegex(exceptions.VnfScaleFailed, - msg, - driver.scale, - self.context, - vnf_info, - scale_vnf_request, - vim_connection_info, - scale_name_list, - grp_id) + self.assertRaisesRegex(exceptions.VnfScaleFailed, msg, + driver._scale_vnf, self.context, vnf_info, + vnf_instance, scale_vnf_request, + vim_connection_info, scale_name_list, grp_id, + op_occ) @mock.patch.object(TackerManager, 'get_service_plugins', return_value={'VNFM': FakeVNFMPlugin()}) @@ -2043,6 +2080,7 @@ class TestVnflcmDriver(db_base.SqlTestCase): @mock.patch.object(driver_manager.DriverManager, "invoke") def test_scale_vim_type_unknown(self, mock_invoke, mock_init_hash, mock_get_service_plugins): + op_occ = mock.MagicMock() mock_init_hash.return_value = { "vnflcm_noop": "ffea638bfdbde3fb01f191bbe75b031859" "b18d663b127100eb72b19eecd7ed51" @@ -2055,20 +2093,22 @@ class TestVnflcmDriver(db_base.SqlTestCase): scale_vnf_request = fakes.scale_request("SCALE_OUT", "SP1", 1, "True") vim_connection_info = vim_connection.VimConnectionInfo( vim_type="unknown") + update = {'vim_connection_info': [vim_connection_info]} + scale_status = objects.ScaleInfo( + aspect_id='vdu1_aspect', scale_level=1) + vnf_instance = fakes.return_vnf_instance( + fields.VnfInstanceState.INSTANTIATED, scale_status=scale_status, + **update) scale_name_list = ["fake"] grp_id = "fake_id" driver = vnflcm_driver.VnfLcmDriver() msg = 'Unknown vim type' - self.assertRaisesRegex(exceptions.VnfScaleFailed, - msg, - driver.scale, - self.context, - vnf_info, - scale_vnf_request, - vim_connection_info, - scale_name_list, - grp_id) + self.assertRaisesRegex(exceptions.VnfScaleFailed, msg, + driver._scale_vnf, self.context, vnf_info, + vnf_instance, scale_vnf_request, + vim_connection_info, scale_name_list, grp_id, + op_occ) @mock.patch.object(TackerManager, 'get_service_plugins', return_value={'VNFM': FakeVNFMPlugin()}) diff --git a/tacker/vnflcm/vnflcm_driver.py b/tacker/vnflcm/vnflcm_driver.py index 86f54df25..d6328d189 100644 --- a/tacker/vnflcm/vnflcm_driver.py +++ b/tacker/vnflcm/vnflcm_driver.py @@ -879,7 +879,6 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): + scale_vnf_request.type) vnf_info['current_error_point'] = EP.VNF_CONFIG_START - scale_id_list = [] scale_name_list = [] grp_id = None vnf_info['policy_name'] = scale_vnf_request.aspect_id @@ -922,7 +921,7 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): scale_vnf_request=scale_vnf_request, grant=vnf_info.get('grant'), grant_request=None, **kwargs) - else: + elif scale_vnf_request.type == 'SCALE_OUT': vnf_info['action'] = 'out' scale_id_list = self._vnf_manager.invoke( vim_connection_info.vim_type, @@ -933,6 +932,10 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): auth_attr=vim_connection_info.access_info, region_name=vim_connection_info.access_info.get('region_name') ) + else: + msg = 'Unknown vim type: %s' % vim_connection_info.vim_type + raise exceptions.VnfScaleFailed(id=vnf_info['instance_id'], + error=msg) vnf_info['current_error_point'] = EP.PRE_VIM_CONTROL return scale_id_list, scale_name_list, grp_id @@ -950,7 +953,6 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): scale_vnf_request, vim_connection_info, scale_id_list, resource_changes): - vnf_lcm_op_occ = vnf_info['vnf_lcm_op_occ'] vnf_info['current_error_point'] = EP.VNF_CONFIG_END if scale_vnf_request.type == 'SCALE_OUT': vnfd_dict = vnflcm_utils._get_vnfd_dict( @@ -966,7 +968,6 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): auth_attr=vim_connection_info.access_info, region_name=vim_connection_info.access_info.get('region_name') ) - id_list = [] id_list = list(set(scale_id_after) - set(scale_id_list)) vnf_info['res_num'] = len(scale_id_after) @@ -1185,15 +1186,6 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): vnf_info['resource_changes'] = resource_changes return resource_changes - def _scale_vnf(self, context, vnf_info, vnf_instance, - scale_vnf_request, vim_connection_info, - scale_name_list, grp_id): - # action_driver - LOG.debug("vnf_info['vnfd']['attributes'] %s", - vnf_info['vnfd']['attributes']) - self.scale(context, vnf_info, scale_vnf_request, - vim_connection_info, scale_name_list, grp_id) - @log.log @revert_to_error_scale def scale_vnf(self, context, vnf_info, vnf_instance, scale_vnf_request): @@ -1225,11 +1217,9 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): vim_connection_info) if vnf_info['before_error_point'] <= EP.POST_VIM_CONTROL: - self._scale_vnf(context, vnf_info, - vnf_instance, - scale_vnf_request, - vim_connection_info, - scale_name_list, grp_id) + self._scale_vnf(context, vnf_info, vnf_instance, scale_vnf_request, + vim_connection_info, scale_name_list, grp_id, + vnf_lcm_op_occ) resource_changes = self._scale_resource_update(context, vnf_info, vnf_instance, @@ -1248,14 +1238,13 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): LOG.info("Request received for scale vnf '%s' is completed " "successfully", vnf_instance.id) - def scale( - self, - context, - vnf_info, - scale_vnf_request, - vim_connection_info, - scale_name_list, - grp_id): + def _scale_vnf(self, context, vnf_info, vnf_instance, scale_vnf_request, + vim_connection_info, scale_name_list, grp_id, + vnf_lcm_op_occ): + # action_driver + LOG.debug("vnf_info['vnfd']['attributes'] %s", (vnf_info + .get('vnfd', {}) + .get('attributes'))) self._vnf_manager = driver_manager.DriverManager( 'tacker.tacker.vnfm.drivers', cfg.CONF.tacker.infra_driver) @@ -1266,10 +1255,11 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): action = 'out' else: msg = 'Unknown scale type: %s' % scale_vnf_request.type - raise exceptions.VnfScaleFailed(id=vnf_info['instance_id'], - error=msg) + raise exceptions.VnfScaleFailed(id=vnf_instance.id, error=msg) - policy = {'instance_id': vnf_info['instance_id'], + stack_id = vnf_instance.instantiated_vnf_info.instance_id + # TODO(h-asahina): change the key name `instance_id` attr to `stack_id` + policy = {'instance_id': stack_id, 'name': scale_vnf_request.aspect_id, 'vnf': vnf_info, 'action': action} @@ -1279,8 +1269,7 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): scale_vnf_request.additional_params.get('is_reverse')) default = None if vim_connection_info.vim_type == 'kubernetes': - policy['vnf_instance_id'] = \ - vnf_info['vnf_lcm_op_occ'].get('vnf_instance_id') + policy['vnf_instance_id'] = vnf_lcm_op_occ.get('vnf_instance_id') vnf_instance = objects.VnfInstance.get_by_id(context, policy['vnf_instance_id']) vnfd_dict = vnflcm_utils._get_vnfd_dict(context, @@ -1306,8 +1295,7 @@ class VnfLcmDriver(abstract_driver.VnfInstanceAbstractDriver): default = scale_group_dict['scaleGroupDict'][key_aspect]['default'] else: msg = 'Unknown vim type: %s' % vim_connection_info.vim_type - raise exceptions.VnfScaleFailed(id=vnf_info['instance_id'], - error=msg) + raise exceptions.VnfScaleFailed(id=vnf_instance.id, error=msg) if (scale_vnf_request.type == 'SCALE_IN' and scale_vnf_request.additional_params['is_reverse'] == 'True'):