From 74bdfde76393764485cfb4667d94589468261dd4 Mon Sep 17 00:00:00 2001 From: Vratko Polak Date: Tue, 13 Aug 2019 10:20:26 +0200 Subject: [PATCH] Revert "Disable CRC checking at runtime" Needed to verify CSIT expects the correct CRCs. This reverts commit d541b2b7d99651b53bd21ff75cd5fdacf8472a98. This reverts commit 2dd27f5a638b5231c0f074ca61e6b67fed9d1faf. + Some pylint related improvements since then. + Less and better documented caching to class fields. + Global kill switch in Constants. Change-Id: Id459800744cd93c578eab9c2e84cb9528235b064 Signed-off-by: Vratko Polak --- resources/libraries/python/Constants.py | 3 ++ resources/libraries/python/PapiExecutor.py | 64 +++++++++++------------------- resources/libraries/python/VppApiCrc.py | 38 +++++++++++++----- 3 files changed, 56 insertions(+), 49 deletions(-) diff --git a/resources/libraries/python/Constants.py b/resources/libraries/python/Constants.py index 0473ca2090..c970d19027 100644 --- a/resources/libraries/python/Constants.py +++ b/resources/libraries/python/Constants.py @@ -109,6 +109,9 @@ class Constants(object): # Default path to VPP API socket. SOCKSVR_PATH = "/run/vpp/api.sock" + # Global "kill switch" for CRC checking during runtime. + CRC_MISMATCH_FAILS_TEST = True + # Mapping from NIC name to its bps limit. # TODO: Implement logic to lower limits to TG NIC or software. Or PCI. NIC_NAME_TO_LIMIT = { diff --git a/resources/libraries/python/PapiExecutor.py b/resources/libraries/python/PapiExecutor.py index 8e59eff510..4e4e0828b0 100644 --- a/resources/libraries/python/PapiExecutor.py +++ b/resources/libraries/python/PapiExecutor.py @@ -128,9 +128,10 @@ class PapiSocketExecutor(object): """ # Class cache for reuse between instances. - cached_vpp_instance = None - api_json_directory = None - crc_checker_instance = None + vpp_instance = None + """Takes long time to create, stores all PAPI functions and types.""" + crc_checker = None + """Accesses .api.json files at creation, caching allows deleting them.""" def __init__(self, node, remote_vpp_socket=Constants.SOCKSVR_PATH): """Store the given arguments, declare managed variables. @@ -148,43 +149,25 @@ class PapiSocketExecutor(object): self._temp_dir = None self._ssh_control_socket = None self._local_vpp_socket = None + self.initialize_vpp_instance() - def create_crc_checker(self): - """Return the cached instance or create new one from directory. + def initialize_vpp_instance(self): + """Create VPP instance with bindings to API calls, store as class field. - It is assumed self.api_json_directory is set, as a class variable. + No-op if the instance had been stored already. - :returns: Cached or newly created instance aware of .api.json content. - :rtype: VppApiCrc.VppApiCrcChecker - """ - cls = self.__class__ - if cls.crc_checker_instance is None: - cls.crc_checker_instance = VppApiCrcChecker(cls.api_json_directory) - return cls.crc_checker_instance - - @property - def vpp_instance(self): - """Return VPP instance with bindings to all API calls. - - The returned instance is initialized for unix domain socket access, + The instance is initialized for unix domain socket access, it has initialized all the bindings, but it is not connected - (to local socket) yet. - - First invocation downloads .api.json files from self._node - into a temporary directory. - - After first invocation, the result is cached, so other calls are quick. - Class variable is used as the cache, but this property is defined as - an instance method, so that _node (for api files) is known. + (to a local socket) yet. - :returns: Initialized but not connected VPP instance. - :rtype: vpp_papi.VPPApiClient + This method downloads .api.json files from self._node + into a temporary directory, deletes them finally. """ - cls = self.__class__ - if cls.cached_vpp_instance is not None: - return cls.cached_vpp_instance + if self.vpp_instance: + return + cls = self.__class__ # Shorthand for setting class fields. tmp_dir = tempfile.mkdtemp(dir="/tmp") - package_path = "Not set yet." + package_path = None try: # Pack, copy and unpack Python part of VPP installation from _node. # TODO: Use rsync or recursive version of ssh.scp_node instead? @@ -201,19 +184,19 @@ class PapiSocketExecutor(object): exec_cmd_no_error(node, ["bash", "-c", "'" + inner_cmd + "'"]) scp_node(node, tmp_dir + "/papi.txz", "/tmp/papi.txz", get=True) run(["tar", "xf", tmp_dir + "/papi.txz", "-C", tmp_dir]) - cls.api_json_directory = tmp_dir + "/usr/share/vpp/api" + api_json_directory = tmp_dir + "/usr/share/vpp/api" # Perform initial checks before .api.json files are gone, - # by accessing the property (which also creates its instance). - self.create_crc_checker() + # by creating the checker instance. + cls.crc_checker = VppApiCrcChecker(api_json_directory) # When present locally, we finally can find the installation path. package_path = glob.glob(tmp_dir + installed_papi_glob)[0] # Package path has to be one level above the vpp_papi directory. package_path = package_path.rsplit('/', 1)[0] sys.path.append(package_path) from vpp_papi.vpp_papi import VPPApiClient as vpp_class - vpp_class.apidir = cls.api_json_directory + vpp_class.apidir = api_json_directory # We need to create instance before removing from sys.path. - cls.cached_vpp_instance = vpp_class( + cls.vpp_instance = vpp_class( use_socket=True, server_address="TBD", async_thread=False, read_timeout=14, logger=FilteredLogger(logger, "INFO")) # Cannot use loglevel parameter, robot.api.logger lacks support. @@ -222,7 +205,6 @@ class PapiSocketExecutor(object): shutil.rmtree(tmp_dir) if sys.path[-1] == package_path: sys.path.pop() - return cls.cached_vpp_instance def __enter__(self): """Create a tunnel, connect VPP instance. @@ -328,7 +310,6 @@ class PapiSocketExecutor(object): run(["ssh", "-S", self._ssh_control_socket, "-O", "exit", "0.0.0.0"], check=False) shutil.rmtree(self._temp_dir) - return def add(self, csit_papi_command, history=True, **kwargs): """Add next command to internal command list; return self. @@ -357,9 +338,11 @@ class PapiSocketExecutor(object): :rtype: PapiSocketExecutor :raises RuntimeError: If unverified or conflicting CRC is encountered. """ + self.crc_checker.report_initial_conflicts() if history: PapiHistory.add_to_papi_history( self._node, csit_papi_command, **kwargs) + self.crc_checker.check_api_name(csit_papi_command) self._api_command_list.append( dict(api_name=csit_papi_command, api_args=copy.deepcopy(kwargs))) return self @@ -519,6 +502,7 @@ class PapiSocketExecutor(object): if not isinstance(reply, list): reply = [reply] for item in reply: + self.crc_checker.check_api_name(item.__class__.__name__) dict_item = dictize(item) if "retval" in dict_item.keys(): # *_details messages do not contain retval. diff --git a/resources/libraries/python/VppApiCrc.py b/resources/libraries/python/VppApiCrc.py index 0d4e8f4f0c..d66d4d7568 100644 --- a/resources/libraries/python/VppApiCrc.py +++ b/resources/libraries/python/VppApiCrc.py @@ -19,6 +19,7 @@ import yaml from robot.api import logger +from resources.libraries.python.Constants import Constants def _str(text): """Convert from possible unicode without interpreting as number. @@ -42,7 +43,8 @@ class VppApiCrcChecker(object): so make sure the calling libraries have appropriate robot library scope. For usual testing, it means "GLOBAL" scope.""" - def __init__(self, directory): + def __init__(self, directory, + fail_on_mismatch=Constants.CRC_MISMATCH_FAILS_TEST): """Initialize empty state, then register known collections. This also scans directory for .api.json files @@ -52,6 +54,11 @@ class VppApiCrcChecker(object): :type directory: str """ + self.fail_on_mismatch = fail_on_mismatch + """If True, mismatch leads to test failure, by raising exception. + If False, the mismatch is logged, but the test is allowed to continue. + """ + self._expected = dict() """Mapping from collection name to mapping from API name to CRC string. @@ -89,6 +96,16 @@ class VppApiCrcChecker(object): self._register_all() self._check_dir(directory) + def raise_or_log(self, exception): + """If fail_on_mismatch, raise, else log to console the exception. + + :param exception: The exception to raise or log. + :type exception: RuntimeError + """ + if self.fail_on_mismatch: + raise exception + logger.console("{exc!r}".format(exc=exception)) + def _register_collection(self, collection_name, name_to_crc_mapping): """Add a named (copy of) collection of CRCs. @@ -243,20 +260,23 @@ class VppApiCrcChecker(object): :param report_missing: Whether to raise on missing messages. :type report_missing: bool - :raises RuntimeError: If CRC mismatch or missing messages are detected. + :raises RuntimeError: If CRC mismatch or missing messages are detected, + and fail_on_mismatch is True. """ if self._initial_conflicts_reported: return self._initial_conflicts_reported = True if self._reported: - raise RuntimeError("Dir check found incompatible API CRCs: {rep!r}"\ - .format(rep=self._reported)) + self.raise_or_log( + RuntimeError("Dir check found incompatible API CRCs: {rep!r}"\ + .format(rep=self._reported))) if not report_missing: return missing = {name: mapp for name, mapp in self._missing.items() if mapp} if missing: - raise RuntimeError("Dir check found missing API CRCs: {mis!r}"\ - .format(mis=missing)) + self.raise_or_log( + RuntimeError("Dir check found missing API CRCs: {mis!r}"\ + .format(mis=missing))) def check_api_name(self, api_name): """Fail if the api_name has no known CRC associated. @@ -285,6 +305,6 @@ class VppApiCrcChecker(object): return crc = self._found.get(api_name, None) self._reported[api_name] = crc - # Disabled temporarily during CRC mismatch. - #raise RuntimeError("No active collection has API {api!r}" - # " CRC found {crc!r}".format(api=api_name, crc=crc)) + self.raise_or_log( + RuntimeError("No active collection has API {api!r}" + " CRC found {crc!r}".format(api=api_name, crc=crc))) -- 2.16.6