Merge ~newell-jensen/maas:lxd-commissioning-hooks-memory into maas:master

[Newell Jensen]

Use LXD commissioning memory output instead of lshw memory output when post processing the script result hooks. Update cpu_speed to iterate over the sockets to take average frequency.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lxd-commissioning-hooks-memory lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6371/console
COMMIT: c9db21b36a34b48c50c200870ffa914d6e5a51fb

review: Needs Fixing

[Lee Trager (ltrager)]

When the CPU frequency isn't found in the CPU model name its value should be the maximum value, not the average.

review: Needs Fixing

24c6cac... by Newell Jensen on 2019-09-10

Review updates.

0e10985... by Newell Jensen on 2019-09-10

Update current average frequency when there is no max.

[Blake Rouse (blake-rouse)]

Looks good!

review: Approve

diff --git a/src/metadataserver/builtin_scripts/hooks.py b/src/metadataserver/builtin_scripts/hooks.py
index 75928e2..e488e44 100644
--- a/src/metadataserver/builtin_scripts/hooks.py
+++ b/src/metadataserver/builtin_scripts/hooks.py
@@ -7,13 +7,11 @@ __all__ = [
     'NODE_INFO_SCRIPTS',
     'parse_lshw_nic_info',
     'update_node_network_information',
-    'lxd_update_cpu_details',
     ]
 
 import fnmatch
 import json
 import logging
-import math
 import re
 
 from lxml import etree
@@ -298,8 +296,8 @@ def get_xml_field_value(evaluator, expression):
 def update_hardware_details(node, output, exit_status):
     """Process the results of `LSHW_SCRIPT`.
 
-    Updates `node.memory`, and `node.storage` fields, and also
-    evaluates all tag expressions against the given ``lshw`` XML.
+    Updates `node.storage` fields, and also evaluates all tag
+    expressions against the given ``lshw`` XML.
 
     If `exit_status` is non-zero, this function returns without doing
     anything.
@@ -318,20 +316,6 @@ def update_hardware_details(node, output, exit_status):
         # Same document, many queries: use XPathEvaluator.
         evaluator = etree.XPathEvaluator(doc)
 
-        # Some machines have a <size> element in their memory <node> with the
-        # total amount of memory, and other machines declare the size of the
-        # memory in individual memory banks. This expression is mean to cope
-        # with both.
-        memory = evaluator("""\
-            sum(//node[@id='memory']/size[@units='bytes'] |
-            //node[starts-with(@id, 'memory:')]
-                /node[starts-with(@id, 'bank:')]/size[@units='bytes'])
-            div 1024 div 1024
-        """)
-        if not memory or math.isnan(memory):
-            memory = 0
-        node.memory = memory
-
         # Only one hardware UUID should be provided but lxml always returns a
         # list.
         for e in evaluator('//node/configuration/setting[@id="uuid"]'):
@@ -339,7 +323,7 @@ def update_hardware_details(node, output, exit_status):
             if value:
                 node.hardware_uuid = value
 
-        node.save(update_fields=['memory', 'hardware_uuid'])
+        node.save(update_fields=['hardware_uuid'])
 
         # This gathers the system vendor, product, version, and serial. Custom
         # built machines and some Supermicro servers do not provide this
@@ -389,15 +373,9 @@ def process_lxd_results(node, output, exit_status):
         raise ValueError(e.message + ': ' + output)
 
     # Update CPU details.
-    lxd_update_cpu_details(node, data)
-
-
-def lxd_update_cpu_details(node, data):
-    """Updates `node.cpu_count`, `node.cpu_speed`, `node.cpu_model`"""
-    # cpu_count, cpu_speed, cpu_model.
     node.cpu_count, node.cpu_speed, cpu_model = _parse_lxd_cpuinfo(data)
-    # memory.
-    node.memory = data.get('memory', {}).get('total', 0)
+    # Update memory.
+    node.memory = data.get('memory', {}).get('total', 0) / 1024 / 1024
 
     if cpu_model:
         NodeMetadata.objects.update_or_create(
@@ -435,19 +413,26 @@ def _parse_lxd_cpuinfo(data):
         if m is not None:
             cpu_speed = int(float(m.group('ghz')) * 1000)
     # When socket names don't match or cpu_speed couldn't be retrieved,
-    # use the max frequency if available before resulting to current
-    # frequency average.
+    # use the max frequency among all the sockets if before
+    # resulting to average current frequency of all the sockets.
     if not cpu_speed:
-        frequency_turbo = socket.get('frequency_turbo')
-        if frequency_turbo is not None:
-            cpu_speed = frequency_turbo
+        max_frequency = 0
+        for socket in sockets:
+            frequency_turbo = socket.get('frequency_turbo', 0)
+            if frequency_turbo > max_frequency:
+                max_frequency = frequency_turbo
+        if max_frequency:
+            cpu_speed = max_frequency
         else:
-            frequency_current_average = socket.get('frequency')
-            if frequency_current_average is not None:
+            current_average = 0
+            for socket in sockets:
+                current_average += socket.get('frequency', 0)
+            current_average /= len(sockets)
+            if current_average:
                 # Fall back on the current speed, round it to
                 # the nearest hundredth as the number may be
                 # effected by CPU scaling.
-                cpu_speed = round(frequency_current_average / 100) * 100
+                cpu_speed = round(current_average / 100) * 100
 
     return cpu_count, cpu_speed, cpu_model
 
diff --git a/src/metadataserver/builtin_scripts/tests/test_hooks.py b/src/metadataserver/builtin_scripts/tests/test_hooks.py
index bb68e1a..870c351 100644
--- a/src/metadataserver/builtin_scripts/tests/test_hooks.py
+++ b/src/metadataserver/builtin_scripts/tests/test_hooks.py
@@ -41,7 +41,7 @@ from metadataserver.builtin_scripts.hooks import (
     extract_router_mac_addresses,
     filter_modaliases,
     get_dmi_data,
-    lxd_update_cpu_details,
+    process_lxd_results,
     retag_node_for_hardware_by_modalias,
     set_virtual_tag,
     SWITCH_OPENBMC_MAC,
@@ -88,7 +88,7 @@ lldp_output_interface_template = """
 """
 
 
-SAMPLE_JSON_CPUINFO = {
+SAMPLE_LXD_JSON = {
     "cpu": {
         "architecture": "x86_64",
         "sockets": [
@@ -135,14 +135,212 @@ SAMPLE_JSON_CPUINFO = {
                             }
                         ],
                         "frequency": 3247
+                    },
+                    {
+                        "core": 1,
+                        "numa_node": 0,
+                        "threads": [
+                            {
+                                "id": 2,
+                                "thread": 0,
+                                "online": True
+                            },
+                            {
+                                "id": 3,
+                                "thread": 1,
+                                "online": True
+                            }
+                        ],
+                        "frequency": 3192
+                    },
+                    {
+                        "core": 2,
+                        "numa_node": 0,
+                        "threads": [
+                            {
+                                "id": 4,
+                                "thread": 0,
+                                "online": True
+                            },
+                            {
+                                "id": 5,
+                                "thread": 1,
+                                "online": True
+                            }
+                        ],
+                        "frequency": 3241
+                    },
+                    {
+                        "core": 3,
+                        "numa_node": 0,
+                        "threads": [
+                            {
+                                "id": 6,
+                                "thread": 0,
+                                "online": True
+                            },
+                            {
+                                "id": 7,
+                                "thread": 1,
+                                "online": True
+                            }
+                        ],
+                        "frequency": 3247
                     }
                 ],
-                "frequency": 3247,
+                "frequency": 3231,
                 "frequency_minimum": 800,
                 "frequency_turbo": 3400
             }
         ],
         "total": 8
+    },
+    "memory": {
+        "nodes": [
+            {
+                "numa_node": 0,
+                "hugepages_used": 0,
+                "hugepages_total": 0,
+                "used": 16430092288,
+                "total": 16691519488
+            }
+        ],
+        "hugepages_total": 0,
+        "hugepages_used": 0,
+        "hugepages_size": 2097152,
+        "used": 12116492288,
+        "total": 16691519488
+    },
+    "gpu": {
+        "cards": [
+            {
+                "driver": "i915",
+                "driver_version": "4.15.0-58-generic",
+                "drm": {
+                    "id": 0,
+                    "card_name": "card0",
+                    "card_device": "226:0",
+                    "control_name": "controlD64",
+                    "control_device": "226:0",
+                    "render_name": "renderD128",
+                    "render_device": "226:128"
+                },
+                "numa_node": 0,
+                "pci_address": "0000:00:02.0",
+                "vendor": "Intel Corporation",
+                "vendor_id": "8086",
+                "product": (
+                    "4th Gen Core Processor Integrated Graphics Controller"),
+                "product_id": "0416"
+            }
+        ],
+        "total": 1
+    },
+    "network": {
+        "cards": [
+            {
+                "driver": "e1000e",
+                "driver_version": "3.2.6-k",
+                "ports": [
+                    {
+                        "id": "enp0s25",
+                        "address": "54:ee:75:13:aa:52",
+                        "port": 0,
+                        "protocol": "ethernet",
+                        "supported_modes": [
+                            "10baseT/Half",
+                            "10baseT/Full",
+                            "100baseT/Half",
+                            "100baseT/Full",
+                            "1000baseT/Full"
+                        ],
+                        "supported_ports": [
+                            "twisted pair"
+                        ],
+                        "port_type": "twisted pair",
+                        "transceiver_type": "internal",
+                        "auto_negotiation": True,
+                        "link_detected": False
+                    }
+                ],
+                "numa_node": 0,
+                "pci_address": "0000:00:19.0",
+                "vendor": "Intel Corporation",
+                "vendor_id": "8086",
+                "product": "Ethernet Connection I217-LM",
+                "product_id": "153a"
+            },
+            {
+                "driver": "iwlwifi",
+                "driver_version": "4.15.0-58-generic",
+                "ports": [
+                    {
+                        "id": "wlp4s0",
+                        "address": "e8:2a:ea:22:0b:d2",
+                        "port": 0,
+                        "protocol": "ethernet",
+                        "auto_negotiation": False,
+                        "link_detected": True
+                    }
+                ],
+                "numa_node": 0,
+                "pci_address": "0000:04:00.0",
+                "vendor": "Intel Corporation",
+                "vendor_id": "8086",
+                "product": "Wireless 7260",
+                "product_id": "08b2"
+            }
+        ],
+        "total": 2
+    },
+    "storage": {
+        "disks": [
+            {
+                "id": "sda",
+                "device": "8:0",
+                "model": "Crucial_CT512M55",
+                "type": "scsi",
+                "read_only": False,
+                "size": 512110190592,
+                "removable": False,
+                "numa_node": 0,
+                "partitions": [
+                    {
+                        "id": "sda1",
+                        "device": "8:1",
+                        "read_only": False,
+                        "size": 536870912,
+                        "partition": 1
+                    },
+                    {
+                        "id": "sda2",
+                        "device": "8:2",
+                        "read_only": False,
+                        "size": 511705088,
+                        "partition": 2
+                    },
+                    {
+                        "id": "sda3",
+                        "device": "8:3",
+                        "read_only": False,
+                        "size": 511060213760,
+                        "partition": 3
+                    }
+                ]
+            },
+            {
+                "id": "sr0",
+                "device": "11:0",
+                "model": "DVD-RAM UJ8E2",
+                "type": "scsi",
+                "read_only": False,
+                "size": 1073741312,
+                "removable": True,
+                "numa_node": 0,
+                "partitions": []
+            }
+        ],
+        "total": 5
     }
 }
 
@@ -726,43 +924,6 @@ class TestUpdateHardwareDetails(MAASServerTestCase):
 
     doctest_flags = doctest.ELLIPSIS | doctest.NORMALIZE_WHITESPACE
 
-    def test_hardware_updates_memory(self):
-        node = factory.make_Node()
-        xmlbytes = dedent("""\
-        <node id="memory">
-           <size units="bytes">4294967296</size>
-        </node>
-        """).encode("utf-8")
-        update_hardware_details(node, xmlbytes, 0)
-        node = reload_object(node)
-        self.assertEqual(4096, node.memory)
-
-    def test_hardware_updates_memory_lenovo(self):
-        node = factory.make_Node()
-        xmlbytes = dedent("""\
-        <node>
-          <node id="memory:0" class="memory">
-            <node id="bank:0" class="memory" handle="DMI:002D">
-              <size units="bytes">4294967296</size>
-            </node>
-            <node id="bank:1" class="memory" handle="DMI:002E">
-              <size units="bytes">3221225472</size>
-            </node>
-          </node>
-          <node id="memory:1" class="memory">
-            <node id="bank:0" class="memory" handle="DMI:002F">
-              <size units="bytes">536870912</size>
-            </node>
-          </node>
-          <node id="memory:2" class="memory"></node>
-        </node>
-        """).encode("utf-8")
-        update_hardware_details(node, xmlbytes, 0)
-        node = reload_object(node)
-        mega = 2 ** 20
-        expected = (4294967296 + 3221225472 + 536879812) // mega
-        self.assertEqual(expected, node.memory)
-
     def test_hardware_updates_hardware_uuid(self):
         node = factory.make_Node()
         hardware_uuid = factory.make_UUID()
@@ -882,53 +1043,54 @@ class TestUpdateHardwareDetails(MAASServerTestCase):
             self.assertIsNone(NodeMetadata.objects.get(node=node, key=key))
 
 
-class TestLXDUpdateCPUDetails(MAASServerTestCase):
+class TestProcessLXDResults(MAASServerTestCase):
 
-    doctest_flags = doctest.ELLIPSIS | doctest.NORMALIZE_WHITESPACE
+    def test__updates_memory(self):
+        node = factory.make_Node()
+        node.memory = random.randint(4096, 8192)
+        node.save()
+
+        process_lxd_results(
+            node, json.dumps(SAMPLE_LXD_JSON).encode('utf-8'), 0)
+        node = reload_object(node)
+        self.assertEqual(round(16691519488 / 1024 / 1024), node.memory)
 
-    def test__speed_in_name(self):
+    def test__updates_model_and_cpu_speed_from_name(self):
         node = factory.make_Node()
-        node.cpu_count = 2
         node.cpu_speed = 9999
         node.save()
 
-        lxd_update_cpu_details(node, SAMPLE_JSON_CPUINFO)
+        process_lxd_results(
+            node, json.dumps(SAMPLE_LXD_JSON).encode('utf-8'), 0)
         node = reload_object(node)
-        self.assertEqual(8, node.cpu_count)
         self.assertEqual(2400, node.cpu_speed)
         nmd = NodeMetadata.objects.get(node=node, key='cpu_model')
         self.assertEqual('Intel(R) Core(TM) i7-4700MQ CPU', nmd.value)
 
-    def test__max_speed(self):
+    def test__updates_cpu_speed_with_max_frequency_when_not_in_name(self):
         node = factory.make_Node()
-        node.cpu_count = 2
         node.cpu_speed = 9999
         node.save()
 
-        SAMPLE_JSON_CPUINFO_NO_SPEED_IN_NAME = deepcopy(SAMPLE_JSON_CPUINFO)
-        SAMPLE_JSON_CPUINFO_NO_SPEED_IN_NAME['cpu']['sockets'][0]['name'] = (
+        NO_SPEED_IN_NAME = deepcopy(SAMPLE_LXD_JSON)
+        NO_SPEED_IN_NAME['cpu']['sockets'][0]['name'] = (
             'Intel(R) Core(TM) i7-4700MQ CPU')
-        lxd_update_cpu_details(node, SAMPLE_JSON_CPUINFO_NO_SPEED_IN_NAME)
+        process_lxd_results(
+            node, json.dumps(NO_SPEED_IN_NAME).encode('utf-8'), 0)
         node = reload_object(node)
-        self.assertEqual(8, node.cpu_count)
         self.assertEqual(3400, node.cpu_speed)
-        nmd = NodeMetadata.objects.get(node=node, key='cpu_model')
-        self.assertEqual('Intel(R) Core(TM) i7-4700MQ CPU', nmd.value)
 
-    def test__current_speed(self):
+    def test__updates_cpu_speed_with_current_frequency_when_not_in_name(self):
         node = factory.make_Node()
-        node.cpu_count = 2
         node.cpu_speed = 9999
         node.save()
 
-        SAMPLE_JSON_CPUINFO_NO_NAME_OR_MAX_FREQ = deepcopy(SAMPLE_JSON_CPUINFO)
-        del SAMPLE_JSON_CPUINFO_NO_NAME_OR_MAX_FREQ[
-            'cpu']['sockets'][0]['name']
-        del SAMPLE_JSON_CPUINFO_NO_NAME_OR_MAX_FREQ[
-            'cpu']['sockets'][0]['frequency_turbo']
-        lxd_update_cpu_details(node, SAMPLE_JSON_CPUINFO_NO_NAME_OR_MAX_FREQ)
+        NO_NAME_OR_MAX_FREQ = deepcopy(SAMPLE_LXD_JSON)
+        del NO_NAME_OR_MAX_FREQ['cpu']['sockets'][0]['name']
+        del NO_NAME_OR_MAX_FREQ['cpu']['sockets'][0]['frequency_turbo']
+        process_lxd_results(
+            node, json.dumps(NO_NAME_OR_MAX_FREQ).encode('utf-8'), 0)
         node = reload_object(node)
-        self.assertEqual(8, node.cpu_count)
         self.assertEqual(3200, node.cpu_speed)