From 98cacdf948facf746503a58263405e29b0956288 Mon Sep 17 00:00:00 2001 From: Glenn Matthews Date: Tue, 17 Jan 2017 12:01:59 -0500 Subject: [PATCH] Fix issue where network name overwrites NIC name --- CHANGELOG.rst | 7 ++- COT/edit_hardware.py | 23 +++++--- COT/ovf/item.py | 29 +++++----- COT/tests/test_edit_hardware.py | 99 ++++++++++++++++++++++++++++++--- docs/thanks.rst | 1 + 5 files changed, 126 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ad7602e..ba1497f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -8,8 +8,11 @@ This project adheres to `Semantic Versioning`_. **Fixed** -- Issue (`#58`_) where various multi-value CLI options for the `edit-hardware` - and `inject-config` commands did not append properly. +- Issue (`#58`_) where various multi-value CLI options for the + ``edit-hardware`` and ``inject-config`` commands did not append properly. +- Issue in which explicitly specified NIC names were being overwritten by + names auto-derived from network names when attempting to set both NIC name + and network names in a single ``cot edit-hardware`` call. **Added** diff --git a/COT/edit_hardware.py b/COT/edit_hardware.py index fd0a936..f347a60 100644 --- a/COT/edit_hardware.py +++ b/COT/edit_hardware.py @@ -3,7 +3,7 @@ # edit_hardware.py - Implements "edit-hardware" sub-command # # September 2013, Glenn F. Matthews -# Copyright (c) 2013-2016 the COT project developers. +# Copyright (c) 2013-2017 the COT project developers. # See the COPYRIGHT.txt file at the top-level directory of this distribution # and at https://github.com/glennmatthews/cot/blob/master/COPYRIGHT.txt. # @@ -447,7 +447,6 @@ def _run_update_nics(self): vm = self.vm nics_dict = vm.get_nic_count(self.profiles) - max_nics = max(nics_dict.values()) if self.nics is not None: for (profile, count) in nics_dict.items(): if self.nics < count: @@ -461,16 +460,9 @@ def _run_update_nics(self): if self.nic_types is not None: vm.set_nic_types(self.nic_types, self.profiles) - nics_dict = vm.get_nic_count(self.profiles) - max_nics = max(nics_dict.values()) - if self.mac_addresses_list is not None: vm.set_nic_mac_addresses(self.mac_addresses_list, self.profiles) - if self.nic_names is not None: - names = expand_list_wildcard(self.nic_names, max_nics) - vm.set_nic_names(names, self.profiles) - def _run_update_networks(self): """Handle network changes. Helper for :meth:`run`.""" vm = self.vm @@ -519,6 +511,15 @@ def _run_update_networks(self): vm.set_nic_networks(new_networks, self.profiles) + def _run_update_nic_names(self): + """Update NIC names. Helper for :meth:`run`.""" + if self.nic_names is not None: + vm = self.vm + nics_dict = vm.get_nic_count(self.profiles) + max_nics = max(nics_dict.values()) + names = expand_list_wildcard(self.nic_names, max_nics) + vm.set_nic_names(names, self.profiles) + def _run_update_serial(self): """Handle serial port changes. Helper for :meth:`run`.""" if self.serial_ports is not None: @@ -573,6 +574,10 @@ def run(self): self._run_update_networks() + # Update NIC names *after* updating networks, as we don't want + # network-induced name changes to overwrite user-specified names. + self._run_update_nic_names() + self._run_update_serial() if self.scsi_subtypes is not None: diff --git a/COT/ovf/item.py b/COT/ovf/item.py index 7594b84..ecdbc8b 100644 --- a/COT/ovf/item.py +++ b/COT/ovf/item.py @@ -3,7 +3,7 @@ # item.py - OVFItem class # # June 2016, Glenn F. Matthews -# Copyright (c) 2013-2016 the COT project developers. +# Copyright (c) 2013-2017 the COT project developers. # See the COPYRIGHT.txt file at the top-level directory of this distribution # and at https://github.com/glennmatthews/cot/blob/master/COPYRIGHT.txt. # @@ -280,10 +280,11 @@ def add_item(self, item): def value_add_wildcards(self, name, value, profiles): """Add wildcard placeholders to a string that may need updating. - If the ElementName or Description references the VirtualQuantity, - Connection, or ResourceSubType, replace that reference with a - placeholder that we can regenerate at output time. That way, if the - VirtualQuantity or ResourceSubType changes, these can change too. + If the Description references the ElementName, or the + ElementName or Description references the VirtualQuantity, + Connection, or ResourceSubType, replace such references with a + placeholder that we can regenerate at output time. That way, if + any of the linked items change, these strings can change too. Args: name (str): Property name @@ -296,6 +297,11 @@ def value_add_wildcards(self, name, value, profiles): .. seealso:: :meth:`value_replace_wildcards` """ + if name == self.ITEM_DESCRIPTION: + en_val = self.get_value(self.ELEMENT_NAME, profiles) + if en_val is not None: + value = re.sub(en_val, "_EN_", value) + if name == self.ELEMENT_NAME or name == self.ITEM_DESCRIPTION: vq_val = self.get_value(self.VIRTUAL_QUANTITY, profiles) if vq_val is not None: @@ -309,11 +315,6 @@ def value_add_wildcards(self, name, value, profiles): if conn_val is not None: value = re.sub(conn_val, "_CONN_", value) - # Similarly, if the Description references the ElementName... - if name == self.ITEM_DESCRIPTION: - en_val = self.get_value(self.ELEMENT_NAME, profiles) - if en_val is not None: - value = re.sub(en_val, "_EN_", value) return value def value_replace_wildcards(self, name, value, profiles): @@ -332,6 +333,10 @@ def value_replace_wildcards(self, name, value, profiles): """ if not value: return value + if name == self.ITEM_DESCRIPTION: + en_val = self._get_value(self.ELEMENT_NAME, profiles) + if en_val is not None: + value = re.sub("_EN_", str(en_val), str(value)) if name == self.ELEMENT_NAME or name == self.ITEM_DESCRIPTION: # To regenerate text that depends on these values: rst_val = self._get_value(self.RESOURCE_SUB_TYPE, profiles) @@ -345,10 +350,6 @@ def value_replace_wildcards(self, name, value, profiles): value = re.sub("_VQ_", str(vq_val), str(value)) if conn_val is not None: value = re.sub("_CONN_", str(conn_val), str(value)) - if name == self.ITEM_DESCRIPTION: - en_val = self._get_value(self.ELEMENT_NAME, profiles) - if en_val is not None: - value = re.sub("_EN_", str(en_val), str(value)) return value def _set_new_property(self, name, value, profiles): diff --git a/COT/tests/test_edit_hardware.py b/COT/tests/test_edit_hardware.py index 96adcd7..5d1a59a 100644 --- a/COT/tests/test_edit_hardware.py +++ b/COT/tests/test_edit_hardware.py @@ -3,7 +3,7 @@ # edit_hardware.py - test cases for the COTEditHardware class # # December 2014, Glenn F. Matthews -# Copyright (c) 2013-2016 the COT project developers. +# Copyright (c) 2013-2017 the COT project developers. # See the COPYRIGHT.txt file at the top-level directory of this distribution # and at https://github.com/glennmatthews/cot/blob/master/COPYRIGHT.txt. # @@ -54,6 +54,23 @@ class TestCOTEditHardware(COT_UT): 'args': ('VM Network',), } + @staticmethod + def removing_network_warning(name=None): + """Warning log message for deleting a network entry. + + Args: + name (str): Name of network being deleted. Defaults to 'VM Network'. + Returns: + dict: kwargs suitable for passing into :meth:`assertLogged` + """ + if not name: + name = "VM Network" + return { + 'levelname': "WARNING", + 'msg': "Removing unused network %s", + 'args': [name], + } + def setUp(self): """Test case setup function called automatically prior to each test.""" super(TestCOTEditHardware, self).setUp() @@ -557,6 +574,72 @@ def test_set_nic_count_add_smart_networks(self): + """, file1=self.csr_ovf) + def test_set_nic_count_named_nics_and_networks(self): + """Add more NICs and explicitly named networks across all profiles. + + This tests a user-reported issue where COT gets confused because the + base OVF uses the same strings for NIC and network names, but the + desired output OVF does not. + """ + self.instance.package = self.csr_ovf + self.instance.nics = 4 + self.instance.nic_names = ['GigabitEthernet{1}'] + self.instance.nic_networks = ["Alpha", "Beta", "Delta", "Gamma"] + self.instance.run() + self.instance.finished() + self.assertLogged(**self.removing_network_warning('GigabitEthernet1')) + self.assertLogged(**self.removing_network_warning('GigabitEthernet2')) + self.assertLogged(**self.removing_network_warning('GigabitEthernet3')) + self.check_diff(""" + The list of logical networks +- +- Data network 1 ++ ++ Alpha + +- +- Data network 2 ++ ++ Beta + +- +- Data network 3 ++ ++ Delta ++ ++ ++ Gamma + +... + true +- GigabitEthernet1 ++ Alpha + NIC representing GigabitEthernet1 +... + true +- GigabitEthernet2 ++ Beta + NIC representing GigabitEthernet2 +... + true +- GigabitEthernet3 ++ Delta + NIC representing GigabitEthernet3 +... + 13 ++ VMXNET3 virtio ++ 10 ++ ++ ++ 14 ++ true ++ Gamma ++ NIC representing GigabitEthernet4 ++ GigabitEthernet4 ++ 14 + VMXNET3 virtio +""", file1=self.csr_ovf) + def test_set_nic_count_merge_profiles(self): """Add NICs that already exist under one profile to another.""" self.instance.package = self.input_ovf @@ -658,7 +741,7 @@ def test_set_nic_count_zero_then_re_add(self): self.instance.nics = 0 self.instance.run() self.instance.finished() - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning('bridged')) self.assertLogged(**self.REMOVING_NETWORKSECTION) self.instance.package = self.temp_file @@ -728,7 +811,7 @@ def test_set_nic_network_all_profiles(self): self.instance.nic_networks = ['UT', 'UT', 'UT'] self.instance.run() self.instance.finished() - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning()) self.check_diff(""" The list of logical networks - @@ -774,7 +857,7 @@ def test_set_nic_network_list_expansion(self): self.instance.network_descriptions = ['First UT'] self.instance.run() self.instance.finished() - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning()) self.check_diff(""" The list of logical networks - @@ -818,7 +901,7 @@ def test_set_nic_network_list_pattern(self): self.instance.network_descriptions = ['First network', '#{2} Network'] self.instance.run() self.instance.finished() - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning()) self.check_diff(""" The list of logical networks - @@ -1071,7 +1154,7 @@ def test_set_nic_kitchen_sink_all_profiles(self): ['00:00:00:00:00:01', '11:22:33:44:55:66', 'fe:fd:fc:fb:fa:f9'] self.instance.run() self.instance.finished() - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning()) self.check_diff(""" The list of logical networks - @@ -1767,7 +1850,7 @@ def test_create_delete_network_no_existing(self): args=('ethernet', 'Connection', ['Foobar'])) self.instance.finished() # network 'Foobar' is not used, so it'll be deleted - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning('Foobar')) self.check_diff(file1=self.minimal_ovf, expected=""" @@ -1804,7 +1887,7 @@ def test_create_delete_network_no_existing(self): self.instance.nics = 0 self.instance.run() self.instance.finished() - self.assertLogged(**self.REMOVING_NETWORK) + self.assertLogged(**self.removing_network_warning()) self.assertLogged(**self.REMOVING_NETWORKSECTION) self.check_diff(file1=self.temp_file, file2=self.minimal_ovf, expected="") diff --git a/docs/thanks.rst b/docs/thanks.rst index bb88116..bc5423b 100644 --- a/docs/thanks.rst +++ b/docs/thanks.rst @@ -15,6 +15,7 @@ We would like to thank: * Jonathan Muslow * Scott O'Donnell * Rick Ogg + * Anantha Padmanabha * Keerthi Rawat * David Rosenfeld * Rafal Skorka