Correct handling of capabilities on node update

When updating nodes we do not convert capabilities from the supported
dict representation to internal string representation. This may result
in incorrect capabilities after an update. This patch fixes it.

Also enforce alphabetical order in capabilities to prevent them
from reordering on updates.

Conflicts:
	tripleo_common/utils/nodes.py

Change-Id: I098409e5ff63b3c3719563de8144fda5bf482bdf
Closes-Bug: #1802318
(cherry picked from commit 2c225f1f2a)

releasenotes/notes/caps-fix-f6f8817a48fa5c25.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Node update now works correctly when capabilities are specified as a dict.

tripleo_common/tests/utils/test_nodes.py

@@ -511,6 +511,52 @@ class NodesTest(base.TestCase):
         nodes._update_or_register_ironic_node(node, node_map, client=ironic)
         ironic.node.update.assert_called_once_with(1, mock.ANY)
 
+    def test_register_update_caps_dict(self):
+        interfaces = {'boot_interface': 'pxe',
+                      'console_interface': 'ipmitool-socat',
+                      'deploy_interface': 'direct',
+                      'inspect_interface': 'inspector',
+                      'management_interface': 'ipmitool',
+                      'network_interface': 'neutron',
+                      'power_interface': 'ipmitool',
+                      'raid_interface': 'agent',
+                      'storage_interface': 'cinder',
+                      'vendor_interface': 'ipmitool'}
+
+        node = self._get_node()
+        node.update(interfaces)
+        node['root_device'] = {'serial': 'abcdef'}
+        node['capabilities'] = {'profile': 'compute', 'num_nics': 6}
+        ironic = mock.MagicMock()
+        node_map = {'mac': {'aaa': 1}}
+
+        def side_effect(*args, **kwargs):
+            update_patch = [
+                {'path': '/name', 'value': 'node1'},
+                {'path': '/driver_info/ipmi_password', 'value': 'random'},
+                {'path': '/driver_info/ipmi_address', 'value': 'foo.bar'},
+                {'path': '/properties/memory_mb', 'value': '2048'},
+                {'path': '/properties/local_gb', 'value': '30'},
+                {'path': '/properties/cpu_arch', 'value': 'amd64'},
+                {'path': '/properties/cpus', 'value': '1'},
+                {'path': '/properties/capabilities',
+                 'value': 'num_nics:6,profile:compute'},
+                {'path': '/properties/root_device',
+                 'value': {'serial': 'abcdef'}},
+                {'path': '/driver_info/ipmi_username', 'value': 'test'}]
+            for iface, value in interfaces.items():
+                update_patch.append({'path': '/%s' % iface, 'value': value})
+            for key in update_patch:
+                key['op'] = 'add'
+            self.assertThat(update_patch,
+                            matchers.MatchesSetwise(*(map(matchers.Equals,
+                                                          args[1]))))
+            return mock.Mock(uuid='uuid1')
+
+        ironic.node.update.side_effect = side_effect
+        nodes._update_or_register_ironic_node(node, node_map, client=ironic)
+        ironic.node.update.assert_called_once_with(1, mock.ANY)
+
     def test_register_update_with_images(self):
         node = self._get_node()
         node['kernel_id'] = 'image-k'

tripleo_common/utils/nodes.py

@@ -451,8 +451,8 @@ _NON_DRIVER_FIELDS = {'cpu': '/properties/cpus',
                       'name': '/name',
                       'resource_class': '/resource_class',
                       'kernel_id': '/driver_info/deploy_kernel',
-                      'ramdisk_id': '/driver_info/deploy_ramdisk',
-                      'capabilities': '/properties/capabilities'}
+                      'ramdisk_id': '/driver_info/deploy_ramdisk'}
+
 
 _NON_DRIVER_FIELDS.update({field: '/%s' % field
                            for field in _KNOWN_INTERFACE_FIELDS})
@@ -471,6 +471,10 @@ def _update_or_register_ironic_node(node, node_map, client):
             if field in node:
                 patched[path] = node.pop(field)
 
+        if 'capabilities' in node:
+            patched['/properties/capabilities'] = dict_to_capabilities(
+                node.pop('capabilities'))
+
         driver_info = handler.convert(node)
         for key, value in driver_info.items():
             patched['/driver_info/%s' % key] = value
@@ -536,6 +540,10 @@ def register_all_nodes(nodes_list, client, remove=False, glance_client=None,
     return seen
 
 
+# These fields are treated specially during enrolling/updating
+_SPECIAL_NON_DRIVER_FIELDS = {'mac', 'pm_type', 'capabilities'}
+
+
 def validate_nodes(nodes_list):
     """Validate all nodes list.
 
@@ -599,7 +607,7 @@ def validate_nodes(nodes_list):
         for field in node:
             converted = handler.convert_key(field)
             if (converted is None and field not in _NON_DRIVER_FIELDS and
-                    field not in ('mac', 'pm_type')):
+                    field not in _SPECIAL_NON_DRIVER_FIELDS):
                 failures.append((index, 'Unknown field %s' % field))
 
     if failures:
@@ -609,8 +617,14 @@ def validate_nodes(nodes_list):
 
 def dict_to_capabilities(caps_dict):
     """Convert a dictionary into a string with the capabilities syntax."""
+    if isinstance(caps_dict, six.string_types):
+        return caps_dict
+
+    # NOTE(dtantsur): sort capabilities so that their order does not change
+    # between updates.
+    items = sorted(caps_dict.items(), key=lambda tpl: tpl[0])
     return ','.join(["%s:%s" % (key, value)
-                     for key, value in caps_dict.items()
+                     for key, value in items
                      if value is not None])