[devices] Clean up platform/os version info for SonicHosts (#1732)

- Deprecate get_asic_type and get_platform_info
- Add facts and os_version properties to SonicHost
- Clarify documentation

Signed-off-by: Danny Allen <[email protected]>

Author: daall

tests/common/broadcom_data.py

@@ -1,2 +1,2 @@
 def is_broadcom_device(dut):
-    return dut.get_asic_type() == "broadcom"
+    return dut.facts["asic_type"] == "broadcom"

tests/common/devices.py

@@ -102,58 +102,103 @@ def __init__(self, ansible_adhoc, hostname):
 
 class SonicHost(AnsibleHostBase):
     """
-    @summary: Class for SONiC switch
+    A remote host running SONiC.
 
-    For running ansible module on the SONiC switch
+    This type of host contains information about the SONiC device (device info, services, etc.),
+    and also provides the ability to run Ansible modules on the SONiC device.
     """
+
+    # TODO: Because people are editing this variable in a bunch of places it should probably be
+    # revised to "DEFAULT_CRITICAL_SERVICES", and we should make critical_services a property of
+    # SonicHost
     CRITICAL_SERVICES = ["swss", "syncd", "database", "teamd", "bgp", "pmon", "lldp", "snmp"]
 
-    def __init__(self, ansible_adhoc, hostname, gather_facts=False):
+    def __init__(self, ansible_adhoc, hostname):
         AnsibleHostBase.__init__(self, ansible_adhoc, hostname)
-        if gather_facts:
-            self.gather_facts()
+        self._facts = self._gather_facts()
+        self._os_version = self._get_os_version()
+
+        if self.facts["num_npu"] > 1:
+            self._update_critical_services_for_multi_npu()
 
-    def _get_critical_services_for_multi_npu(self):
+    @property
+    def facts(self):
         """
-        Update the critical_services with the service names for multi-npu platforms
+        Platform information for this SONiC device.
+
+        Returns:
+            dict: A dictionary containing the device platform information.
+
+            For example:
+            {
+                "platform": "x86_64-arista_7050_qx32s",
+                "hwsku": "Arista-7050-QX-32S",
+                "asic_type": "broadcom",
+                "num_npu": 1
+            }
         """
-        m_service = []
-        for service in self.CRITICAL_SERVICES:
-            for npu in self.facts["num_npu"]:
-                npu_service = service+npu
-                m_service.insert(npu, npu_service)
-        self.CRITICAL_SERVICES = m_service
-        print self.CRITICAL_SERVICES
 
-    def _get_npu_info(self):
+        return self._facts
+
+    @property
+    def os_version(self):
+        """
+        The OS version running on this SONiC device.
+        
+        Returns:
+            str: The SONiC OS version (e.g. "20181130.31")
+        """
+
+        return self._os_version
+
+    def _gather_facts(self):
+        """
+        Gather facts about the platform for this SONiC device.
+        """
+
+        facts = dict()
+        facts.update(self._get_platform_info())
+        facts["num_npu"] = self._get_npu_count(facts["platform"])
+
+        logging.debug("Gathered SonicHost facts: %s" % json.dumps(facts))
+        return facts
+
+    def _get_npu_count(self, platform):
         """
-        Check if the DUT is multi-npu platfrom and store the number of npus in the facts
+        Gets the number of npus for this device.
         """
-        asic_conf_file_path = os.path.join('/usr/share/sonic/device', self.facts["platform"], 'asic.conf')
+
+        asic_conf_file_path = os.path.join("/usr/share/sonic/device", platform, "asic.conf")
         try:
-            output = self.shell('cat %s' % asic_conf_file_path)["stdout_lines"]
-            print output
+            output = self.shell("cat {}".format(asic_conf_file_path))["stdout_lines"]
+            logging.debug(output)
+
             for line in output:
-                num_npu=line.split("=",1)[1].strip()
-            print "num_npu = {}".format(num_npu)
-            self.facts["num_npu"] = int(num_npu)
+                num_npu = line.split("=", 1)[1].strip()
+
+            logging.debug("num_npu = %s" % num_npu)
+            return int(num_npu)
         except:
-            self.facts["num_npu"] =1
+            return 1
 
-        if self.facts["num_npu"] > 1:
-            self._get_critical_services_for_multi_npu
+    def _update_critical_services_for_multi_npu(self):
+        """
+        Updates the critical services for this device with the services for multi-npu platforms.
+        """
 
+        m_service = []
+        for service in self.CRITICAL_SERVICES:
+            for npu in self.facts["num_npu"]:
+                npu_service = service + npu
+                m_service.insert(npu, npu_service)
+        self.CRITICAL_SERVICES = m_service
+        logging.debug(self.CRITICAL_SERVICES)
 
-    def get_platform_info(self):
+    def _get_platform_info(self):
         """
-        @summary: Get the platform information of the SONiC switch.
-        @return: Returns a dictionary containing preperties of the platform information, for example:
-            {
-                "platform": "",
-                "hwsku": "",
-                "asic_type": ""
-            }
+        Gets platform information about this SONiC device.
         """
+
         platform_info = self.command("show platform summary")["stdout_lines"]
         result = {}
         for line in platform_info:
@@ -165,15 +210,13 @@ def get_platform_info(self):
                 result["asic_type"] = line.split(":")[1].strip()
         return result
 
-    def gather_facts(self):
+    def _get_os_version(self):
         """
-        @summary: Gather facts of the SONiC switch and store the gathered facts in the dict type 'facts' attribute.
+        Gets the SONiC OS version that is running on this device.
         """
-        self.facts = {}
-        platform_info = self.get_platform_info()
-        self.facts.update(platform_info)
-        self._get_npu_info()
-        logging.debug("SonicHost facts: %s" % json.dumps(self.facts))
+
+        output = self.command("sonic-cfggen -y /etc/sonic/sonic_version.yml -v build_version")
+        return output["stdout_lines"][0].strip()
 
     def get_service_props(self, service, props=["ActiveState", "SubState"]):
         """
@@ -424,9 +467,6 @@ def get_image_info(self):
         ret['installed_list'] = images
         return ret
 
-    def get_asic_type(self):
-        return self.facts["asic_type"]
-
     def shutdown(self, ifname):
         """
             Shutdown interface specified by ifname
@@ -557,16 +597,6 @@ def check_bgp_session_nsf(self, neighbor_ip):
                 return True
         return False
 
-    def get_version(self):
-        """
-            Gets the SONiC version this device is running.
-
-            Returns:
-                str: the firmware version number (e.g. 20181130.31)
-        """
-        output = dut.command("sonic-cfggen -y /etc/sonic/sonic_version.yml -v build_version")
-        return output["stdout_lines"][0].strip()
-
 class EosHost(AnsibleHostBase):
     """
     @summary: Class for Eos switch

tests/common/mellanox_data.py

@@ -552,4 +552,4 @@
 }
 
 def is_mellanox_device(dut):
-    return dut.get_asic_type() == "mellanox"
+    return dut.facts["asic_type"] == "mellanox"

tests/common/system_utils/docker.py

@@ -130,7 +130,7 @@ def swap_syncd(dut):
     elif is_mellanox_device(dut):
         vendor_id = "mlnx"
     else:
-        error_message = "\"{}\" is not currently supported".format(dut.get_asic_type())
+        error_message = "\"{}\" is not currently supported".format(dut.facts["asic_type"])
         _LOGGER.error(error_message)
         raise ValueError(error_message)
 
@@ -176,7 +176,7 @@ def restore_default_syncd(dut):
     elif is_mellanox_device(dut):
         vendor_id = "mlnx"
     else:
-        error_message = "\"{}\" is not currently supported".format(dut.get_asic_type())
+        error_message = "\"{}\" is not currently supported".format(dut.facts["asic_type"])
         _LOGGER.error(error_message)
         raise ValueError(error_message)
 

tests/conftest.py

@@ -214,7 +214,7 @@ def duthost(ansible_adhoc, testbed, request):
     dut_index = getattr(request.module, "dut_index", 0)
     assert dut_index < len(testbed["duts"]), "DUT index '{0}' is out of bound '{1}'".format(dut_index, len(testbed["duts"]))
 
-    duthost = SonicHost(ansible_adhoc, testbed["duts"][dut_index], gather_facts=True)
+    duthost = SonicHost(ansible_adhoc, testbed["duts"][dut_index])
     if stop_ssh_timeout is not None:
         disable_ssh_timout(duthost)
 

tests/copp/test_copp.py

@@ -62,7 +62,7 @@ def test_policer(self, protocol, duthost, ptfhost, _copp_testbed):
 
         if protocol == "ARP" \
                 and is_broadcom_device(duthost) \
-                and "201811" not in duthost.get_version():
+                and "201811" not in duthost.os_version:
             pytest.xfail("ARP policy disabled on BRCM devices due to SAI bug")
 
         if protocol in ["IP2ME", "SNMP", "SSH"] and _copp_testbed.topo == "t1-lag":

tests/platform_tests/test_sensors.py

@@ -5,7 +5,7 @@
 
 def test_sensors(duthost, creds):
     # Get platform name
-    platform = duthost.get_platform_info()['platform']
+    platform = duthost.facts['platform']
 
     # Prepare check list
     sensors_checks = creds['sensors_checks']