Fix issue where network name overwrites NIC name

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**
 

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:

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):

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):
 +      </ovf:Item>
      </ovf:VirtualHardwareSection>""", 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("""
+     <ovf:Info>The list of logical networks</ovf:Info>
+-    <ovf:Network ovf:name="GigabitEthernet1">
+-      <ovf:Description>Data network 1</ovf:Description>
++    <ovf:Network ovf:name="Alpha">
++      <ovf:Description>Alpha</ovf:Description>
+     </ovf:Network>
+-    <ovf:Network ovf:name="GigabitEthernet2">
+-      <ovf:Description>Data network 2</ovf:Description>
++    <ovf:Network ovf:name="Beta">
++      <ovf:Description>Beta</ovf:Description>
+     </ovf:Network>
+-    <ovf:Network ovf:name="GigabitEthernet3">
+-      <ovf:Description>Data network 3</ovf:Description>
++    <ovf:Network ovf:name="Delta">
++      <ovf:Description>Delta</ovf:Description>
++    </ovf:Network>
++    <ovf:Network ovf:name="Gamma">
++      <ovf:Description>Gamma</ovf:Description>
+     </ovf:Network>
+...
+         <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
+-        <rasd:Connection>GigabitEthernet1</rasd:Connection>
++        <rasd:Connection>Alpha</rasd:Connection>
+         <rasd:Description>NIC representing GigabitEthernet1</rasd:Description>
+...
+         <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
+-        <rasd:Connection>GigabitEthernet2</rasd:Connection>
++        <rasd:Connection>Beta</rasd:Connection>
+         <rasd:Description>NIC representing GigabitEthernet2</rasd:Description>
+...
+         <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
+-        <rasd:Connection>GigabitEthernet3</rasd:Connection>
++        <rasd:Connection>Delta</rasd:Connection>
+         <rasd:Description>NIC representing GigabitEthernet3</rasd:Description>
+...
+         <rasd:InstanceID>13</rasd:InstanceID>
++        <rasd:ResourceSubType>VMXNET3 virtio</rasd:ResourceSubType>
++        <rasd:ResourceType>10</rasd:ResourceType>
++      </ovf:Item>
++      <ovf:Item>
++        <rasd:AddressOnParent>14</rasd:AddressOnParent>
++        <rasd:AutomaticAllocation>true</rasd:AutomaticAllocation>
++        <rasd:Connection>Gamma</rasd:Connection>
++        <rasd:Description>NIC representing GigabitEthernet4</rasd:Description>
++        <rasd:ElementName>GigabitEthernet4</rasd:ElementName>
++        <rasd:InstanceID>14</rasd:InstanceID>
+         <rasd:ResourceSubType>VMXNET3 virtio</rasd:ResourceSubType>
+""", 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("""
      <ovf:Info>The list of logical networks</ovf:Info>
 -    <ovf:Network ovf:name="VM Network">
@@ -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("""
      <ovf:Info>The list of logical networks</ovf:Info>
 -    <ovf:Network ovf:name="VM Network">
@@ -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("""
      <ovf:Info>The list of logical networks</ovf:Info>
 -    <ovf:Network ovf:name="VM Network">
@@ -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("""
      <ovf:Info>The list of logical networks</ovf:Info>
 -    <ovf:Network ovf:name="VM Network">
@@ -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="""
  <?xml version='1.0' encoding='utf-8'?>
@@ -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="")

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