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.

Change-Id: I098409e5ff63b3c3719563de8144fda5bf482bdf
Closes-Bug: #1802318

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

@@ -549,6 +549,53 @@ 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',
+                      'rescue_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

@@ -490,7 +490,6 @@ _NON_DRIVER_FIELDS = {'cpu': '/properties/cpus',
                                     '/driver_info/rescue_kernel'],
                       'ramdisk_id': ['/driver_info/deploy_ramdisk',
                                      '/driver_info/rescue_ramdisk'],
-                      'capabilities': '/properties/capabilities',
                       'platform': '/extra/tripleo_platform',
                       }
 
@@ -516,6 +515,10 @@ def _update_or_register_ironic_node(node, node_map, client):
                 for path in paths:
                     patched[path] = value
 
+        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
@@ -581,6 +584,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 = {'ports', 'pm_type', 'capabilities'}
+
+
 def validate_nodes(nodes_list):
     """Validate all nodes list.
 
@@ -650,7 +657,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 ('ports', 'pm_type')):
+                    field not in _SPECIAL_NON_DRIVER_FIELDS):
                 failures.append((index, 'Unknown field %s' % field))
 
     if failures:
@@ -660,8 +667,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])