search

ibm-openbmc / openpower-vpd-parser

Library and parser for OpenPower-format VPD
Apache License 2.0
3 stars 20 forks source link

In worker::parseVpdFile API, check if PostFailAction is present in JSON before executing it #441

Closed PriyangaRamasamy closed 3 weeks ago

PriyangaRamasamy commented 1 month ago

For DIMMs we don't need PostFailAction tag in JSON.

If DIMM slots are empty, vpd-manager tries to execute preAction and if preAction fails, without checking PostFailAction tag presence, vpd-manager worker::parseVpdFile API attempts to executePostFailActon api which throws unwanted logs and adds up execution time on a system which has only 2 dimms out of 64 dimms.

Oct 24 07:24:38 ever6bmc vpd-manager[1733]: FileName: /usr/src/debug/openpower-fru-vpd/1.0+git/include/utility/json_utility.hpp, Line: 212, Func: bool vpd::jsonUtility::executePostFailAction(const nlohmann::json_abi_v3_11_3::json&, const std::string&, const std::string&), PostFailAction flag missing in config JSON. Abort processing

Oct 24 07:24:38 ever6bmc vpd-manager[1733]: FileName: /usr/src/debug/openpower-fru-vpd/1.0+git/src/worker.cpp, Line: 1232, Func: std::tuple<bool, std::__cxx11::basic_string<char, std::char_traits, std::allocator > > vpd::Worker::parseAndPublishVPD(const std::string&), Post fail action failed for path /sys/bus/i2c/drivers/at24/810-0050/eeprom Aborting collection for this FRU

souvik1914581 commented 3 weeks ago

@PriyangaRamasamy , I agree that this issue results in "unwanted logs" from FRUs which do not have PostFailAction defined in the JSON, but how will adding a check for PostFailAction tag reduce execution time?

Right now, we execute PostFailAction for a FRU in two scenarios:

  1. If the preAction is successful but the VPD file path doesn't exist.
  2. If the preAction is successful, but the VPD parsing fails.

In both the above scenarios, we go into jsonUtility::executePostFailAction() which fails to find PostFailAction tag in the parsed system JSON and returns false.

Suppose, we add a check for PostFailAction, we would need to call jsonUtility::isActionRequired() once processPreAction() is successful. jsonUtility::isActionRequired() again checks the parsed system JSON for PostFailAction tag, so the execution time would be the same as in the present day code? Only benefit of adding this check would be the logs wouldn't flood, but as Jinu says we are going to do away with logs in the future.

PriyangaRamasamy commented 3 weeks ago

I agree that this issue results in "unwanted logs" from FRUs which do not have PostFailAction defined in the JSON, but how will adding a check for PostFailAction tag reduce execution time?

unwanted logs (62log traces on everest which has only 2 dimms present) getting logged to journal messages adds execution time.

PriyangaRamasamy commented 3 weeks ago

Only benefit of adding this check would be the logs wouldn't flood

no. If tag not found we throw error/log a PEL/. That shouldn't be the case as postFailAction tag is not a mandatory one in JSON. Similar to how we did for other json tags, we need to check if postFailAction tag is present in JSON before calling the executePostFailAction() api.

souvik1914581 commented 3 weeks ago

no. If tag not found we throw error/log a PEL/. That shouldn't be the case as postFailAction tag is not a mandatory one in JSON. Similar to how we did for other json tags, we need to check if postFailAction tag is present in JSON before calling the executePostFailAction() api.

Ok, I agree with this.

souvik1914581 commented 3 weeks ago

Addressed by https://github.com/ibm-openbmc/openpower-vpd-parser/pull/454 Closing this issue.