From fc2e9e21066592781f703ab6626999412769dc23 Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Date: Sat, 12 Jun 2021 00:26:37 +0800 Subject: [PATCH] [eeprom_tlv_info] Optimize EEPROM data process by using visitor pattern (#193) Remove EEPROM cache file and use DB instead 1. Use visitor pattern accessing EEPROM data 2. Provide utility functions to access redis data base --- .gitignore | 2 + .../sonic_eeprom/eeprom_base.py | 17 ++ .../sonic_eeprom/eeprom_tlvinfo.py | 289 +++++++++++------- 3 files changed, 204 insertions(+), 104 deletions(-) diff --git a/.gitignore b/.gitignore index c5fcca1580f5..19b34b7720c6 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,5 @@ sonic_platform_common.egg-info/ # Unit test / coverage reports .coverage htmlcov/ +coverage.xml +test-results.xml \ No newline at end of file diff --git a/sonic_platform_base/sonic_eeprom/eeprom_base.py b/sonic_platform_base/sonic_eeprom/eeprom_base.py index 05d74573c4fa..083d1cd1f6be 100644 --- a/sonic_platform_base/sonic_eeprom/eeprom_base.py +++ b/sonic_platform_base/sonic_eeprom/eeprom_base.py @@ -27,6 +27,8 @@ def __init__(self, path, format, start, status, readonly): self.s = start self.u = status self.r = readonly + # Warning: the following members are deprecated, the parsed EEPROM data is stored in the + # Redis STATE_DB, cached data should be fetched from STATE_DB.EEPROM_INFO. self.cache_name = None self.cache_update_needed = False self.lock_file = None @@ -47,6 +49,9 @@ def check_status(self): return 'ok' def set_cache_name(self, name): + # Warning: this method is deprecated, the parsed EEPROM data is stored in the + # Redis STATE_DB, cached data should be fetched from STATE_DB.EEPROM_INFO. + # before accessing the eeprom we acquire an exclusive lock on the eeprom file. # this will prevent a race condition where multiple instances of this app # could try to update the cache at the same time @@ -214,6 +219,9 @@ def open_eeprom(self): using_eeprom = True eeprom_file = self.p try: + # Warning: cache file is deprecated, the parsed EEPROM data is stored in the + # Redis STATE_DB, cached data should be fetched from STATE_DB.EEPROM_INFO. This + # code need to be adjusted once cache file is completely removing from the system. if os.path.isfile(self.cache_name): eeprom_file = self.cache_name using_eeprom = False @@ -240,6 +248,9 @@ def read_eeprom_bytes(self, byteCount, offset=0): # expect, the file may be corrupt. Delete it and try again, this # time reading from the actual EEPROM. if len(o) != byteCount and not self.cache_update_needed: + # Warning: cache file is deprecated, the parsed EEPROM data is stored in the + # Redis STATE_DB, cached data should be fetched from STATE_DB.EEPROM_INFO. This + # code needs to be adjusted once cache file is completely removed from the system. os.remove(self.cache_name) self.cache_update_needed = True F.close() @@ -277,6 +288,9 @@ def write_eeprom(self, e): self.write_cache(e) def write_cache(self, e): + # Warning: this method is deprecated, the parsed EEPROM data is stored in the + # Redis STATE_DB, cached data should be fetched from STATE_DB.EEPROM_INFO. + if self.cache_name: F = None try: @@ -290,6 +304,9 @@ def write_cache(self, e): F.close() def update_cache(self, e): + # Warning: this method is deprecated, the parsed EEPROM data is stored in the + # Redis STATE_DB, cached data should be fetched from STATE_DB.EEPROM_INFO. + if self.cache_update_needed: self.write_cache(e) fcntl.flock(self.lock_file, fcntl.LOCK_UN) diff --git a/sonic_platform_base/sonic_eeprom/eeprom_tlvinfo.py b/sonic_platform_base/sonic_eeprom/eeprom_tlvinfo.py index 2a6e18dfe58f..b7070d8a646f 100644 --- a/sonic_platform_base/sonic_eeprom/eeprom_tlvinfo.py +++ b/sonic_platform_base/sonic_eeprom/eeprom_tlvinfo.py @@ -79,22 +79,23 @@ def __init__(self, path, start, status, ro, max_len=_TLV_INFO_MAX_LEN): ro) self.eeprom_start = start self.eeprom_max_len = max_len + self._redis_client = None - def __print_db(self, db, code, num=0): + def __print_db(self, code, num=0): if not num: - field_name = db.hget('EEPROM_INFO|{}'.format(hex(code)), 'Name') + field_name = self._redis_hget('EEPROM_INFO|{}'.format(hex(code)), 'Name') if not field_name: pass else: - field_len = db.hget('EEPROM_INFO|{}'.format(hex(code)), 'Len') - field_value = db.hget('EEPROM_INFO|{}'.format(hex(code)), 'Value') + field_len = self._redis_hget('EEPROM_INFO|{}'.format(hex(code)), 'Len') + field_value = self._redis_hget('EEPROM_INFO|{}'.format(hex(code)), 'Value') print("%-20s 0x%02X %3s %s" % (field_name, code, field_len, field_value)) else: for index in range(num): - field_name = db.hget('EEPROM_INFO|{}'.format(hex(code)), 'Name_{}'.format(index)) - field_len = db.hget('EEPROM_INFO|{}'.format(hex(code)), 'Len_{}'.format(index)) - field_value = db.hget('EEPROM_INFO|{}'.format(hex(code)), 'Value_{}'.format(index)) + field_name = self._redis_hget('EEPROM_INFO|{}'.format(hex(code)), 'Name_{}'.format(index)) + field_len = self._redis_hget('EEPROM_INFO|{}'.format(hex(code)), 'Len_{}'.format(index)) + field_value = self._redis_hget('EEPROM_INFO|{}'.format(hex(code)), 'Value_{}'.format(index)) print("%-20s 0x%02X %3s %s" % (field_name, code, field_len, field_value)) @@ -102,35 +103,8 @@ def decode_eeprom(self, e): ''' Decode and print out the contents of the EEPROM. ''' - if self._TLV_HDR_ENABLED : - if not self.is_valid_tlvinfo_header(e): - print("EEPROM does not contain data in a valid TlvInfo format.") - return - - print("TlvInfo Header:") - print(" Id String: %s" % e[0:7].decode("ascii")) - print(" Version: %d" % e[8]) - total_len = (e[9] << 8) | e[10] - print(" Total Length: %d" % total_len) - tlv_index = self._TLV_INFO_HDR_LEN - tlv_end = self._TLV_INFO_HDR_LEN + total_len - else : - tlv_index = self.eeprom_start - tlv_end = self._TLV_INFO_MAX_LEN - - print("TLV Name Code Len Value") - print("-------------------- ---- --- -----") - while (tlv_index + 2) < len(e) and tlv_index < tlv_end: - if not self.is_valid_tlv(e[tlv_index:]): - print("Invalid TLV field starting at EEPROM offset %d" % tlv_index) - return - tlv = e[tlv_index:tlv_index + 2 + e[tlv_index + 1]] - name, value = self.decoder(None, tlv) - print("%-20s 0x%02X %3d %s" % (name, tlv[0], tlv[1], value)) - if e[tlv_index] == self._TLV_CODE_QUANTA_CRC or \ - e[tlv_index] == self._TLV_CODE_CRC_32: - return - tlv_index += e[tlv_index+1] + 2 + visitor = EepromDecodeVisitor() + self.visit_eeprom(e, visitor) def set_eeprom(self, e, cmd_args): @@ -306,35 +280,34 @@ def read_eeprom_db(self): ''' Print out the contents of the EEPROM from database ''' - client = redis.Redis(db = STATE_DB_INDEX) - db_state = client.hget('EEPROM_INFO|State', 'Initialized') + db_state = self._redis_hget('EEPROM_INFO|State', 'Initialized') if db_state != '1': return -1 - tlv_version = client.hget('EEPROM_INFO|TlvHeader', 'Version') + tlv_version = self._redis_hget('EEPROM_INFO|TlvHeader', 'Version') if tlv_version: print("TlvInfo Header:") - print(" Id String: %s" % client.hget('EEPROM_INFO|TlvHeader', 'Id String')) + print(" Id String: %s" % self._redis_hget('EEPROM_INFO|TlvHeader', 'Id String')) print(" Version: %s" % tlv_version) - print(" Total Length: %s" % client.hget('EEPROM_INFO|TlvHeader', 'Total Length')) + print(" Total Length: %s" % self._redis_hget('EEPROM_INFO|TlvHeader', 'Total Length')) print("TLV Name Code Len Value") print("-------------------- ---- --- -----") for index in range(self._TLV_CODE_PRODUCT_NAME, self._TLV_CODE_SERVICE_TAG + 1): - self.__print_db(client, index) + self.__print_db(index) try: - num_vendor_ext = int(client.hget('EEPROM_INFO|{}'.format(hex(self._TLV_CODE_VENDOR_EXT)), 'Num_vendor_ext')) + num_vendor_ext = int(self._redis_hget('EEPROM_INFO|{}'.format(hex(self._TLV_CODE_VENDOR_EXT)), 'Num_vendor_ext')) except (ValueError, TypeError): pass else: - self.__print_db(client, self._TLV_CODE_VENDOR_EXT, num_vendor_ext) + self.__print_db(self._TLV_CODE_VENDOR_EXT, num_vendor_ext) - self.__print_db(client, self._TLV_CODE_CRC_32) + self.__print_db(self._TLV_CODE_CRC_32) print("") - is_valid = client.hget('EEPROM_INFO|Checksum', 'Valid') + is_valid = self._redis_hget('EEPROM_INFO|Checksum', 'Valid') if is_valid != '1': print("(*** checksum invalid)") else: @@ -347,64 +320,10 @@ def update_eeprom_db(self, e): ''' Decode the contents of the EEPROM and update the contents to database ''' - client = redis.Redis(db=STATE_DB_INDEX) - fvs = {} - if self._TLV_HDR_ENABLED: - if not self.is_valid_tlvinfo_header(e): - print("EEPROM does not contain data in a valid TlvInfo format.") - return -1 - total_len = (e[9] << 8) | e[10] - fvs['Id String'] = e[0:7].decode("ascii") - fvs['Version'] = e[8] - fvs['Total Length'] = total_len - client.hmset("EEPROM_INFO|TlvHeader", fvs) - fvs.clear() - tlv_index = self._TLV_INFO_HDR_LEN - tlv_end = self._TLV_INFO_HDR_LEN + total_len - else: - tlv_index = self.eeprom_start - tlv_end = self._TLV_INFO_MAX_LEN - - vendor_ext_tlv_num = 0 - while (tlv_index + 2) < len(e) and tlv_index < tlv_end: - if not self.is_valid_tlv(e[tlv_index:]): - break - tlv = e[tlv_index:tlv_index + 2 + e[tlv_index + 1]] - tlv_code = tlv[0] - if tlv_code == self._TLV_CODE_VENDOR_EXT: - vendor_index = str(vendor_ext_tlv_num) - fvs['Len_{}'.format(vendor_index)] = tlv[1] - fvs['Name_{}'.format(vendor_index)], fvs['Value_{}'.format(vendor_index)] = self.decoder(None, tlv) - vendor_ext_tlv_num += 1 - else: - fvs['Len'] = tlv[1] - fvs['Name'], fvs['Value'] = self.decoder(None, tlv) - client.hmset('EEPROM_INFO|{}'.format(hex(tlv_code)), fvs) - fvs.clear() - if e[tlv_index] == self._TLV_CODE_QUANTA_CRC or \ - e[tlv_index] == self._TLV_CODE_CRC_32: - break - else: - tlv_index += e[tlv_index + 1] + 2 - - if vendor_ext_tlv_num > 0: - fvs['Num_vendor_ext'] = str(vendor_ext_tlv_num) - client.hmset('EEPROM_INFO|{}'.format(hex(self._TLV_CODE_VENDOR_EXT)), fvs) - fvs.clear() - - (is_valid, valid_crc) = self.is_checksum_valid(e) - if is_valid: - fvs['Valid'] = '1' - else: - fvs['Valid'] = '0' - - client.hmset('EEPROM_INFO|Checksum', fvs) - fvs.clear() - - fvs['Initialized'] = '1' - client.hmset('EEPROM_INFO|State', fvs) - return 0 - + visitor = EepromRedisVisitor(self) + self.visit_eeprom(e, visitor) + return 0 if visitor.error is None else -1 + def get_tlv_field(self, e, code): ''' @@ -695,3 +614,165 @@ def checksum_field_size(self): def checksum_type(self): return 'crc32' + + @property + def redis_client(self): + """Handy property to get a redis client. Make sure only create the redis client once. + + Returns: + A redis client instance + """ + if not self._redis_client: + self._redis_client = redis.Redis(db=STATE_DB_INDEX) + return self._redis_client + + def _redis_hget(self, key, field): + value = self.redis_client.hget(key, field) + if value is not None: + value = value.decode().rstrip('\0') + return value + + def visit_eeprom(self, e, visitor): + """Visit the content of EEPROM data. + + Args: + e: the EEPROM data + visitor: A instance of EEPROM vistor, see an example at EepromDefaultVisitor + """ + if self._TLV_HDR_ENABLED : + if not self.is_valid_tlvinfo_header(e): + visitor.set_error('EEPROM does not contain data in a valid TlvInfo format.') + return + + total_len = (e[9] << 8) | e[10] + tlv_index = self._TLV_INFO_HDR_LEN + tlv_end = self._TLV_INFO_HDR_LEN + total_len + visitor.visit_header(e[0:7].decode("ascii"), int(e[8]), total_len) + else : + tlv_index = self.eeprom_start + tlv_end = self._TLV_INFO_MAX_LEN + visitor.visit_header(None, None, None) + + while (tlv_index + 2) < len(e) and tlv_index < tlv_end: + if not self.is_valid_tlv(e[tlv_index:]): + visitor.set_error('Invalid TLV field starting at EEPROM offset %d' % tlv_index) + break + tlv = e[tlv_index:tlv_index + 2 + e[tlv_index + 1]] + name, value = self.decoder(None, tlv) + visitor.visit_tlv(name, tlv[0], tlv[1], value) + + if e[tlv_index] == self._TLV_CODE_QUANTA_CRC or \ + e[tlv_index] == self._TLV_CODE_CRC_32: + break + tlv_index += e[tlv_index+1] + 2 + visitor.visit_end(e) + + +class EepromDefaultVisitor: + def visit_header(self, eeprom_id, version, header_length): + """Visit EEPROM header. If EEPROM header is not present, this method is still called, + but the input arguments will all be None. + + Args: + eeprom_id (str): EEPROM ID + version (int): EEPROM version + header_length (int): EEPROM header length + """ + pass + + def visit_tlv(self, name, code, length, value): + """Visit a TLV + + Args: + name (str): name of the TLV + code (int): code of the TLV + length (int): value length of the TLV + value (str): TLV value + """ + pass + + def visit_end(self, eeprom_data): + """The method will be called when visit EEPROM data finish + + Args: + eeprom_data: EEPROM data + """ + pass + + def set_error(self, error): + """Set error when visiting EEPROM data + + Args: + error (str): error message + """ + pass + + +class EepromDecodeVisitor(EepromDefaultVisitor): + def visit_header(self, eeprom_id, version, header_length): + if eeprom_id is not None: + print("TlvInfo Header:") + print(" Id String: %s" % eeprom_id) + print(" Version: %d" % version) + print(" Total Length: %d" % header_length) + print("TLV Name Code Len Value") + print("-------------------- ---- --- -----") + + def visit_tlv(self, name, code, length, value): + print("%-20s 0x%02X %3d %s" % (name, code, length, value)) + + def set_error(self, error): + print(error) + + +class EepromRedisVisitor(EepromDefaultVisitor): + def __init__(self, eeprom_object): + self.eeprom_object = eeprom_object + self.redis_client = eeprom_object.redis_client + self.vendor_ext_tlv_num = 0 + self.fvs = {} + self.error = None + + def visit_header(self, eeprom_id, version, header_length): + if eeprom_id is not None: + self.fvs['Id String'] = eeprom_id + self.fvs['Version'] = version + self.fvs['Total Length'] = header_length + self.redis_client.hmset("EEPROM_INFO|TlvHeader", self.fvs) + self.fvs.clear() + + def visit_tlv(self, name, code, length, value): + if code == self.eeprom_object._TLV_CODE_VENDOR_EXT: + vendor_index = str(self.vendor_ext_tlv_num) + self.fvs['Name_{}'.format(vendor_index)] = name + self.fvs['Len_{}'.format(vendor_index)] = length + self.fvs['Value_{}'.format(vendor_index)] = value + self.vendor_ext_tlv_num += 1 + else: + self.fvs['Name'] = name + self.fvs['Len'] = length + self.fvs['Value'] = value + self.redis_client.hmset('EEPROM_INFO|{}'.format(hex(code)), self.fvs) + self.fvs.clear() + + def visit_end(self, eeprom_data): + if self.vendor_ext_tlv_num > 0: + self.fvs['Num_vendor_ext'] = str(self.vendor_ext_tlv_num) + self.redis_client.hmset('EEPROM_INFO|{}'.format(hex(self.eeprom_object._TLV_CODE_VENDOR_EXT)), self.fvs) + self.fvs.clear() + + (is_valid, _) = self.eeprom_object.is_checksum_valid(eeprom_data) + if is_valid: + self.fvs['Valid'] = '1' + else: + self.fvs['Valid'] = '0' + + self.redis_client.hmset('EEPROM_INFO|Checksum', self.fvs) + self.fvs.clear() + + self.fvs['Initialized'] = '1' + self.redis_client.hmset('EEPROM_INFO|State', self.fvs) + + def set_error(self, error): + self.error = error + print(error)