From 81092e6d7c9e31783e7a73d28bc50c6d131df3d3 Mon Sep 17 00:00:00 2001 From: Ian H Pittwood Date: Wed, 31 Jul 2019 15:42:25 -0500 Subject: [PATCH] Address TODO notes in engine This change addresses some of the TODOs made in the Spyglass engine. There will be additional follow-up patchsets that will address issues with the rules engine and intermediary validation. Change-Id: Iba70a51d291659bf827e46fc9070a898303082d1 --- spyglass/cli.py | 4 +- spyglass/parser/engine.py | 91 ++++++++------------------------ tests/unit/parser/test_engine.py | 64 ++++++---------------- 3 files changed, 41 insertions(+), 118 deletions(-) diff --git a/spyglass/cli.py b/spyglass/cli.py index 726fb04..fb1c5fc 100644 --- a/spyglass/cli.py +++ b/spyglass/cli.py @@ -134,8 +134,8 @@ def intermediary_processor(plugin_type, **kwargs): # Apply design rules to the data LOG.info("Apply design rules to the extracted data") - process_input_ob = ProcessDataSource(kwargs['site_name']) - process_input_ob.load_extracted_data_from_data_source(data_extractor.data) + process_input_ob = ProcessDataSource( + kwargs['site_name'], data_extractor.data) return process_input_ob diff --git a/spyglass/parser/engine.py b/spyglass/parser/engine.py index c48a0f9..f869502 100755 --- a/spyglass/parser/engine.py +++ b/spyglass/parser/engine.py @@ -28,10 +28,19 @@ LOG = logging.getLogger(__name__) class ProcessDataSource(object): - def __init__(self, site_type): + def __init__(self, region, extracted_data): # Initialize intermediary and save site type - self._initialize_intermediary() - self.region_name = site_type + self.host_type = {} + self.sitetype = None + self.genesis_node = None + self.network_subnets = None + self.region_name = region + + LOG.info("Loading plugin data source") + self.data = extracted_data + LOG.debug( + "Extracted data from plugin:\n{}".format( + pprint.pformat(extracted_data))) @staticmethod def _read_file(file_name): @@ -39,21 +48,6 @@ class ProcessDataSource(object): raw_data = f.read() return raw_data - def _initialize_intermediary(self): - # TODO(ian-pittwood): Define these in init, remove this function - self.host_type = {} - self.data = { - "network": {}, - "baremetal": {}, - "region_name": "", - "storage": {}, - "site_info": {}, - } - self.sitetype = None - self.genesis_node = None - self.region_name = None - self.network_subnets = None - def _get_network_subnets(self): """Extract subnet information for networks. @@ -73,13 +67,9 @@ class ProcessDataSource(object): return network_subnets def _get_genesis_node_details(self): - # TODO(ian-pittwood): Use get_baremetal_host_by_type instead - # TODO(ian-pittwood): Below should be docstring, not comment - # Get genesis host node details from the hosts based on host type - for rack in self.data.baremetal: - for host in rack.hosts: - if host.type == "genesis": - self.genesis_node = host + """Get genesis host node details from the hosts based on host type""" + for host in self.data.get_baremetal_host_by_type('genesis'): + self.genesis_node = host LOG.debug( "Genesis Node Details:\n{}".format( pprint.pformat(self.genesis_node))) @@ -142,37 +132,22 @@ class ProcessDataSource(object): """ LOG.info("Apply design rules") - # TODO(ian-pittwood): Use more robust path creation methods such - # as os.path.join. We may also want to let - # users specify these in cli opts. We also need - # better guidelines over how to write these rules - # and how they are applied. + # TODO(ian-pittwood): We may want to let users specify these in cli + # opts. We also need better guidelines over how + # to write these rules and how they are applied. rules_dir = resource_filename("spyglass", "config/") - rules_file = rules_dir + "rules.yaml" + rules_file = os.path.join(rules_dir, "rules.yaml") rules_data_raw = self._read_file(rules_file) rules_yaml = yaml.safe_load(rules_data_raw) - rules_data = {} - rules_data.update(rules_yaml) - for rule in rules_data.keys(): - rule_name = rules_data[rule]["name"] + for rule in rules_yaml.keys(): + rule_name = rules_yaml[rule]["name"] function_str = "_apply_rule_" + rule_name - rule_data_name = rules_data[rule][rule_name] + rule_data_name = rules_yaml[rule][rule_name] function = getattr(self, function_str) function(rule_data_name) LOG.info("Applying rule:{}".format(rule_name)) - def _apply_rule_host_profile_interfaces(self, rule_data): - # TODO(pg710r)Nothing to do as of now since host profile - # information is already present in plugin data. - # This function shall be defined if plugin data source - # doesn't provide host profile information. - - # TODO(ian-pittwood): Should be implemented as it is outside of - # our plugin packages. Logic can be implemented - # to ensure proper data processing. - pass - def _apply_rule_hardware_profile(self, rule_data): """Apply rules to define host type from hardware profile info. @@ -225,7 +200,6 @@ class ProcessDataSource(object): host_idx = 0 LOG.info("Update baremetal host ip's") - # TODO(ian-pittwood): this can be redone to be cleaner with models for rack in self.data.baremetal: for host in rack.hosts: for net_type, net_ip in iter(host.ip): @@ -310,27 +284,6 @@ class ProcessDataSource(object): "Updated vlan network data:\n{}".format( pprint.pformat(vlan_network_data_.dict_from_class()))) - def load_extracted_data_from_data_source(self, extracted_data): - # TODO(ian-pittwood): Remove this and use init - - """Function called from cli.py to pass extracted data - - from input data source - """ - - LOG.info("Loading plugin data source") - self.data = extracted_data - LOG.debug( - "Extracted data from plugin:\n{}".format( - pprint.pformat(extracted_data))) - - # Uncomment following segment for debugging purpose. - # extracted_file = "extracted_file.yaml" - # yaml_file = yaml.dump(extracted_data, default_flow_style=False) - # with open(extracted_file, 'w') as f: - # f.write(yaml_file) - # f.close() - def dump_intermediary_file(self, intermediary_dir): """Writing intermediary yaml""" diff --git a/tests/unit/parser/test_engine.py b/tests/unit/parser/test_engine.py index 112fa74..fa3082b 100644 --- a/tests/unit/parser/test_engine.py +++ b/tests/unit/parser/test_engine.py @@ -32,12 +32,14 @@ class TestProcessDataSource(unittest.TestCase): REGION_NAME = 'test' def test___init__(self): - with mock.patch.object( - ProcessDataSource, - '_initialize_intermediary') as mock__initialize_intermediary: - obj = ProcessDataSource(self.REGION_NAME) + expected_data = 'data' + obj = ProcessDataSource(self.REGION_NAME, expected_data) self.assertEqual(self.REGION_NAME, obj.region_name) - mock__initialize_intermediary.assert_called_once() + self.assertDictEqual({}, obj.host_type) + self.assertEqual(expected_data, obj.data) + self.assertIsNone(obj.sitetype) + self.assertIsNone(obj.genesis_node) + self.assertIsNone(obj.network_subnets) def test__read_file(self): test_file = os.path.join(FIXTURE_DIR, 'site_config.yaml') @@ -46,24 +48,6 @@ class TestProcessDataSource(unittest.TestCase): data = ProcessDataSource._read_file(test_file) self.assertEqual(expected_data, data) - def test__initialize_intermediary(self): - expected_data = { - "network": {}, - "baremetal": {}, - "region_name": "", - "storage": {}, - "site_info": {}, - } - - obj = ProcessDataSource(self.REGION_NAME) - obj._initialize_intermediary() - self.assertDictEqual({}, obj.host_type) - self.assertDictEqual(expected_data, obj.data) - self.assertIsNone(obj.sitetype) - self.assertIsNone(obj.genesis_node) - self.assertIsNone(obj.region_name) - self.assertIsNone(obj.network_subnets) - def test__get_network_subnets(self): expected_result = { 'calico': IPNetwork('30.29.1.0/25'), @@ -73,8 +57,7 @@ class TestProcessDataSource(unittest.TestCase): 'pxe': IPNetwork('30.30.4.0/25'), 'storage': IPNetwork('30.31.1.0/25') } - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) result = obj._get_network_subnets() self.assertDictEqual(expected_result, result) @@ -82,8 +65,7 @@ class TestProcessDataSource(unittest.TestCase): expected_result = self.site_document_data.get_baremetal_host_by_type( 'genesis')[0] - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) obj._get_genesis_node_details() self.assertEqual(expected_result, obj.genesis_node) @@ -95,22 +77,16 @@ class TestProcessDataSource(unittest.TestCase): @mock.patch.object(ProcessDataSource, '_apply_rule_hardware_profile') def test__apply_design_rules( self, mock_rule_hw_profile, mock_rule_ip_alloc_offset): - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) obj._apply_design_rules() mock_rule_hw_profile.assert_called_once() mock_rule_ip_alloc_offset.assert_called_once() - @unittest.skip('Not implemented.') - def test__apply_rule_host_profiles_interfaces(self): - pass - def test__apply_rule_hardware_profile(self): input_rules = self.rules_data['rule_hardware_profile'][ 'hardware_profile'] - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) obj._apply_rule_hardware_profile(input_rules) self.assertEqual( 1, len(obj.data.get_baremetal_host_by_type('genesis'))) @@ -132,8 +108,7 @@ class TestProcessDataSource(unittest.TestCase): def test__apply_rule_ip_alloc_offset( self, mock__get_network_subnets, mock__update_vlan_net_data, mock__update_baremetal_host_ip_data): - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) obj._apply_rule_ip_alloc_offset(self.rules_data) self.assertEqual('success', obj.network_subnets) mock__get_network_subnets.assert_called_once() @@ -142,8 +117,7 @@ class TestProcessDataSource(unittest.TestCase): self.rules_data) def test__update_baremetal_host_ip_data(self): - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) obj.network_subnets = obj._get_network_subnets() ip_alloc_offset_rules = self.rules_data['rule_ip_alloc_offset'][ 'ip_alloc_offset'] @@ -163,8 +137,7 @@ class TestProcessDataSource(unittest.TestCase): ip_alloc_offset_rules = self.rules_data['rule_ip_alloc_offset'][ 'ip_alloc_offset'] - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) obj.network_subnets = obj._get_network_subnets() obj._update_vlan_net_data(ip_alloc_offset_rules) @@ -211,14 +184,12 @@ class TestProcessDataSource(unittest.TestCase): self.assertEqual([], vlan.routes) def test_load_extracted_data_from_data_source(self): - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) self.assertEqual(self.site_document_data, obj.data) @mock.patch('yaml.dump', return_value='success') def test_dump_intermediary_file(self, mock_dump): - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) mock_open = mock.mock_open() with mock.patch('spyglass.parser.engine.open', mock_open): obj.dump_intermediary_file(None) @@ -232,8 +203,7 @@ class TestProcessDataSource(unittest.TestCase): @mock.patch.object(ProcessDataSource, '_get_genesis_node_details') def test_generate_intermediary_yaml( self, mock__apply_design_rules, mock__get_genesis_node_details): - obj = ProcessDataSource(self.REGION_NAME) - obj.load_extracted_data_from_data_source(self.site_document_data) + obj = ProcessDataSource(self.REGION_NAME, self.site_document_data) result = obj.generate_intermediary_yaml() self.assertEqual(self.site_document_data, result) mock__apply_design_rules.assert_called_once()