Added consistency checker to all relevant advanced-reboot cases (#16877)

Description of PR
Summary: Added consistency checker to all relevant advanced-reboot cases and added minor improvements including:

Ignoring NULL values in ASIC querying
Logging ASIC/DB inconsistencies
Fixes # (issue)

Author: Ryangwaite

tests/common/fixtures/advanced_reboot.py

@@ -91,6 +91,8 @@ def __init__(self, request, duthosts, duthost, ptfhost, localhost, tbinfo, creds
         self.allowMacJump = kwargs["allow_mac_jumping"] if "allow_mac_jumping" in kwargs else False
         self.advanceboot_loganalyzer = kwargs["advanceboot_loganalyzer"] if "advanceboot_loganalyzer"\
                                                                             in kwargs else None
+        self.consistency_checker_provider = kwargs["consistency_checker_provider"] if "consistency_checker_provider"\
+                                                                                      in kwargs else None
         self.other_vendor_nos = kwargs['other_vendor_nos'] if 'other_vendor_nos' in kwargs else False
         self.__dict__.update(kwargs)
         self.__extractTestParam()
@@ -557,6 +559,50 @@ def acl_manager_checker(self, error_list):
         if int(acl_proc_count) != 1:
             error_list.append("Expected one ACL manager process running. Actual: {}".format(acl_proc_count))
 
+    def check_asic_and_db_consistency(self):
+        """
+        Check ASIC_DB and ASIC consistency, logging out any inconsistencies that are found.
+        """
+        if not self.consistency_checker_provider.is_consistency_check_supported(self.duthost):
+            os_version = self.duthost.image_facts()["ansible_facts"]["ansible_image_facts"]["current"]
+            platform = self.duthost.facts['platform']
+            logger.info((f"Consistency check is not supported on this platform ({platform}) and "
+                        f"version ({os_version})"))
+            return
+
+        with self.consistency_checker_provider.get_consistency_checker(self.duthost) as consistency_checker:
+            inconsistencies = consistency_checker.check_consistency()
+            not_implemented_attributes = set()
+            mismatched_attributes = {}
+            failed_to_query_asic_attributes = {}
+
+            for sai_object, summary in inconsistencies.items():
+                # Not implemented attributes
+                object_name = sai_object.split(":")[1]
+                for attr in summary["attributeNotImplemented"]:
+                    not_implemented_attributes.add(f"{object_name}.{attr}")
+
+                # Mismatched attributes
+                mismatched_attributes = {
+                    attr: summary["attributes"][attr] for attr
+                    in summary["mismatchedAttributes"]
+                }
+                if mismatched_attributes:
+                    mismatched_attributes[sai_object] = mismatched_attributes
+
+                # Failed to query ASIC attributes
+                if summary["failedToQueryAsic"]:
+                    failed_to_query_asic_attributes[sai_object] = summary["failedToQueryAsic"]
+
+            if not_implemented_attributes:
+                logger.warning(f"Not implemented attributes: {not_implemented_attributes}")
+
+            if mismatched_attributes:
+                logger.error(f"Mismatched attributes found: {mismatched_attributes}")
+
+            if failed_to_query_asic_attributes:
+                logger.error(f"Failed to query ASIC attributes: {failed_to_query_asic_attributes}")
+
     def runRebootTest(self):
         # Run advanced-reboot.ReloadTest for item in preboot/inboot list
         count = 0
@@ -597,6 +643,8 @@ def runRebootTest(self):
             finally:
                 if self.postboot_setup:
                     self.postboot_setup()
+                if self.consistency_checker_provider:
+                    self.check_asic_and_db_consistency()
                 # capture the test logs, and print all of them in case of failure, or a summary in case of success
                 log_dir = self.__fetchTestLogs(rebootOper, log_dst_suffix=rebootOper)
                 self.print_test_logs_summary(log_dir)
@@ -677,6 +725,9 @@ def runMultiHopRebootTest(self, upgrade_path_urls, rebootOper=None, base_image_s
                             self.duthost.hostname, upgrade_path_str))
                 if post_hop_teardown:
                     post_hop_teardown(hop_index)
+
+                if self.consistency_checker_provider:
+                    self.check_asic_and_db_consistency()
             except Exception:
                 traceback_msg = traceback.format_exc()
                 err_msg = "Exception caught while running advanced-reboot test on ptf during upgrade {}: \n{}".format(

tests/common/fixtures/consistency_checker/consistency_checker.py

@@ -3,8 +3,10 @@
 import json
 import os
 import datetime
+from typing import List, Optional
 from collections import defaultdict
-from tests.common.fixtures.consistency_checker.constants import SUPPORTED_PLATFORMS_AND_VERSIONS
+from tests.common.fixtures.consistency_checker.constants import SUPPORTED_PLATFORMS_AND_VERSIONS, \
+    ConsistencyCheckQueryKey, ALL_ATTRIBUTES
 
 logger = logging.getLogger(__name__)
 
@@ -154,7 +156,7 @@ def get_db_and_asic_peers(self, keys=["*"]) -> dict:
 
         return dict(results)
 
-    def check_consistency(self, keys=["*"]) -> dict:
+    def check_consistency(self, keys=None) -> dict:
         """
         Get the out-of-sync ASIC_DB and ASIC attributes. Differences are indicative of an error state.
         Same arg style as the get_objects function but returns a list of objects that don't match or couldn't
@@ -163,7 +165,7 @@ def check_consistency(self, keys=["*"]) -> dict:
 
         :param keys: Optional list of glob search strings that correspond to the --key arg of sonic-db-dump.
                      sonic-db-dump doesn't take multiple keys, so a list is passed in to support multiple
-                     keys at the API level.
+                     keys at the API level. If not provided, then the default keys are used.
         :return: Dictionary containing the out-of-sync ASIC_DB and ASIC attributes.
 
         Example return val (matching):
@@ -186,19 +188,25 @@ def check_consistency(self, keys=["*"]) -> dict:
                     "failedToQueryAsic": [
                         {"SAI_BUFFER_PROFILE_ATTR_SHARED_DYNAMIC_TH": "Failed to query attribute value"}
                     ],
-                    "mismatchedAttributes": ["SAI_BUFFER_PROFILE_ATTR_THRESHOLD_MODE"]
+                    "mismatchedAttributes": ["SAI_BUFFER_PROFILE_ATTR_THRESHOLD_MODE"],
+                    "attributeNotImplemented": ["SAI_BUFFER_PROFILE_ATTR_POOL_ID"]
                 },
                 ...
             }
         """
+        if keys is None:
+            platform = self._duthost.facts['platform']
+            os_version = self._duthost.image_facts()["ansible_facts"]["ansible_image_facts"]["current"]
+            keys = self._get_consistency_checker_keys(platform, os_version)
 
         db_attributes = self._get_db_attributes(keys)
         asic_attributes = self._get_asic_attributes_from_db_results(db_attributes)
 
         inconsistencies = defaultdict(lambda: {
             "attributes": {},
             "failedToQueryAsic": [],
-            "mismatchedAttributes": []
+            "mismatchedAttributes": [],
+            "attributeNotImplemented": [],
         })
 
         for object in db_attributes:
@@ -231,23 +239,71 @@ def check_consistency(self, keys=["*"]) -> dict:
                 if asic_query_success:
                     inconsistencies[object]["mismatchedAttributes"].append(attr)
                 else:
-                    inconsistencies[object]["failedToQueryAsic"].append({attr: asic_object[attr]["error"]})
+                    error = asic_object[attr]["error"]
+                    if "ATTR_NOT_IMPLEMENTED" in error:
+                        inconsistencies[object]["attributeNotImplemented"].append(attr)
+                    else:
+                        inconsistencies[object]["failedToQueryAsic"].append({attr: error})
 
         return dict(inconsistencies)
 
-    def _get_db_attributes(self, keys: list) -> dict:
+    def _get_consistency_checker_keys(self, platform, os_version) -> List[str]:
+        """
+        Get the keys for the given platform and OS version.
+
+        :param platform: Platform name
+        :param os_version: OS version
+        :return: List of keys
+        """
+
+        if platform not in SUPPORTED_PLATFORMS_AND_VERSIONS:
+            raise Exception(f"Unsupported platform: {platform}")
+
+        supported_versions = SUPPORTED_PLATFORMS_AND_VERSIONS[platform]
+        for version in supported_versions:
+            if version in os_version:
+                return supported_versions[version]
+
+        raise Exception(f"Unsupported OS version: {os_version}")
+
+    def _get_db_attributes(self, keys: List[ConsistencyCheckQueryKey]) -> dict:
         """
         Fetchs and merges the attributes of the objects returned by the search key from the DB.
         """
         db_attributes = {}
         for key in keys:
-            result = self._duthost.command(f"sonic-db-dump -k '{key}' -n ASIC_DB")
+            result = self._duthost.command(f"sonic-db-dump -k '{key.key}' -n ASIC_DB")
             if result['rc'] != 0:
                 raise Exception((f"Failed to fetch attributes for key '{key}' from ASIC_DB. "
                                  f"Return code: {result['rc']}, stdout: {result['stdout']}, "
                                  f"stderr: {result['stderr']}"))
 
             query_result = json.loads(result['stdout'])
+
+            # Filter for attributes that we want ...
+            objects_with_no_attrs = []
+            for object in query_result:
+
+                if "NULL" in query_result[object]["value"]:
+                    logger.debug(f"Ignoring attribute 'NULL' for object '{object}'")
+                    del query_result[object]["value"]["NULL"]
+
+                if ALL_ATTRIBUTES in key.attributes:
+                    logger.debug(f"Retaining all attributes for object '{object}'")
+                else:
+                    attributes_to_remove = set(query_result[object]["value"].keys()) - set(key.attributes)
+                    for attr in attributes_to_remove:
+                        logger.debug(f"Ignoring attribute '{attr}' for object '{object}'")
+                        del query_result[object]["value"][attr]
+
+                if len(query_result[object]["value"]) == 0:
+                    objects_with_no_attrs.append(object)
+
+            # ... then remove the objects that have no attributes left
+            for object in objects_with_no_attrs:
+                logger.debug(f"Ignoring empty object '{object}'")
+                del query_result[object]
+
             db_attributes.update(query_result)
 
         return db_attributes
@@ -304,6 +360,19 @@ def _get_asic_attributes_from_db_results(self, db_attributes: dict) -> dict:
 
 
 class ConsistencyCheckerProvider:
+
+    def __init__(self, libsairedis_url_template: Optional[str],
+                 python3_pysairedis_url_template:  Optional[str]) -> None:
+        """
+        The libsairedis_url_template and python3_pysairedis_url_template are optional URL templates that the
+        consistency checker can use to download the libsairedis and python3-pysairedis debs respectively.
+
+        :param libsairedis_url_template: Optional URL template for the libsairedis deb
+        :param python3_pysairedis_url_template: Optional URL template for the python3-pysairedis deb
+        """
+        self._libsairedis_url_template = libsairedis_url_template
+        self._python3_pysairedis_url_template = python3_pysairedis_url_template
+
     def is_consistency_check_supported(self, dut) -> bool:
         """
         Checks if the provided DUT is supported for consistency checking.
@@ -318,29 +387,56 @@ def is_consistency_check_supported(self, dut) -> bool:
 
         current_version = dut.image_facts()['ansible_facts']['ansible_image_facts']['current']
         supported_versions = SUPPORTED_PLATFORMS_AND_VERSIONS[platform]
-        if any(v in current_version for v in supported_versions):
+        if any(v in current_version for v in supported_versions.keys()):
             return True
 
         return False
 
-    def get_consistency_checker(self, dut, libsairedis_download_url=None,
-                                python3_pysairedis_download_url=None) -> ConsistencyChecker:
+    def get_consistency_checker(self, dut) -> ConsistencyChecker:
         """
         Get a new instance of the ConsistencyChecker class.
 
         :param dut: SonicHost object
-        :param libsairedis_download_url: Optional URL that the consistency checker should use to download the
-               libsairedis deb
-        :param python3_pysairedis_download_url: Optional URL that the consistency checker should use to
-               download the python3-pysairedis deb
         :return ConsistencyChecker: New instance of the ConsistencyChecker class
         """
+
+        os_version = dut.image_facts()["ansible_facts"]["ansible_image_facts"]["current"]
+
+        if self._libsairedis_url_template or self._python3_pysairedis_url_template:
+            if "202305" in os_version:
+                sonic_version_template_param = "202305"
+            elif "202311" in os_version:
+                sonic_version_template_param = "202311"
+            else:
+                raise Exception(f"Unsupported OS version: {os_version}")
+
+        libsairedis_download_url = self._libsairedis_url_template\
+            .format(sonic_version=sonic_version_template_param)\
+            if self._libsairedis_url_template else None
+
+        python3_pysairedis_download_url = self._python3_pysairedis_url_template\
+            .format(sonic_version=sonic_version_template_param)\
+            if self._python3_pysairedis_url_template else None
+
         return ConsistencyChecker(dut, libsairedis_download_url, python3_pysairedis_download_url)
 
 
 @pytest.fixture
-def consistency_checker_provider():
+def consistency_checker_provider(request):
     """
     Fixture that provides the ConsistencyCheckerProvider class.
+
+    :param request: pytest request object
     """
-    return ConsistencyCheckerProvider()
+
+    if not request.config.getoption("enable_consistency_checker"):
+        logger.info("Consistency checker is not enabled. Skipping check.")
+        return None
+
+    consistency_checker_libsairedis_url_template = request.config.getoption(
+        "consistency_checker_libsairedis_url_template")
+    consistency_checker_python3_pysairedis_url_template = request.config.getoption(
+        "consistency_checker_python3_pysairedis_url_template")
+
+    return ConsistencyCheckerProvider(consistency_checker_libsairedis_url_template,
+                                      consistency_checker_python3_pysairedis_url_template)

tests/common/fixtures/consistency_checker/constants.py

@@ -1,6 +1,51 @@
+from dataclasses import dataclass
+from typing import List
+
+ALL_ATTRIBUTES = "all"
+
+
+@dataclass
+class ConsistencyCheckQueryKey:
+    key: str
+    attributes: List[str]
+
+
+BROADCOM_KEYS: List[ConsistencyCheckQueryKey] = [
+    ConsistencyCheckQueryKey("ASIC_STATE:SAI_OBJECT_TYPE_BUFFER_POOL:*", attributes=[ALL_ATTRIBUTES]),
+    ConsistencyCheckQueryKey("ASIC_STATE:SAI_OBJECT_TYPE_BUFFER_PROFILE:*", attributes=[ALL_ATTRIBUTES]),
+    ConsistencyCheckQueryKey("ASIC_STATE:SAI_OBJECT_TYPE_SWITCH:*", attributes=[ALL_ATTRIBUTES]),
+    ConsistencyCheckQueryKey("ASIC_STATE:SAI_OBJECT_TYPE_WRED:*", attributes=[ALL_ATTRIBUTES]),
+    ConsistencyCheckQueryKey(
+        "ASIC_STATE:SAI_OBJECT_TYPE_PORT:*",
+        attributes=[
+            "SAI_PORT_ATTR_QOS_TC_TO_QUEUE_MAP",
+            "SAI_PORT_ATTR_QOS_TC_TO_PRIORITY_GROUP_MAP",
+            "SAI_PORT_ATTR_QOS_PFC_PRIORITY_TO_QUEUE_MAP",
+            "SAI_PORT_ATTR_QOS_DSCP_TO_TC_MAP",
+            "SAI_PORT_ATTR_MTU",
+            "SAI_PORT_ATTR_INGRESS_ACL",
+            "SAI_PORT_ATTR_AUTO_NEG_MODE",
+            "SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL",
+            "SAI_PORT_ATTR_ADMIN_STATE",
+            "SAI_PORT_ATTR_FEC_MODE",
+            # The "get" implementation of the SAI_PORT_ATTR_SPEED attribute sometimes has a side effect of changing
+            # the port speed. Consistency-checker should not change the state of the DUT, so we ignore this attribute
+            # "SAI_PORT_ATTR_SPEED",
+            # This attribute doesn't match between ASIC_DB and ASIC SAI and the test fails the assertion
+            # "SAI_PORT_ATTR_PORT_VLAN_ID",
+        ]
+    ),
+]
+
 
 # The list of platforms and versions that have been tested to work with the consistency checker
 SUPPORTED_PLATFORMS_AND_VERSIONS = {
-    "x86_64-arista_7060_cx32s": ["202305", "202311"],
-    "x86_64-arista_7260cx3_64": ["202305", "202311"],
+    "x86_64-arista_7060_cx32s": {
+        "202305": BROADCOM_KEYS,
+        "202311": BROADCOM_KEYS,
+    },
+    "x86_64-arista_7260cx3_64": {
+        "202305": BROADCOM_KEYS,
+        "202311": BROADCOM_KEYS,
+    },
 }