worker::CollectionStatus for each inventory FRU #547

PriyangaRamasamy · 2024-12-18T12:18:41Z

This commit implements CollectionStatus D-bus property under com.ibm.VPD.Collection D-bus interface for each inventory D-bus object path which represents a FRU.

The property tells the current status of VPD collection for a given FRU's D-bus object path.

The property takes the below enum values:

com.ibm.VPD.Collection.Status.Success
-------------------------------------
This value is assigned when VPD collection is successful.

com.ibm.VPD.Collection.Status.Failure

This value is assigned when we hit prime inventory path on VPD collection failure due to VPD exceptions and also when single FRU VPD collection fails.

com.ibm.VPD.Collection.Status.InProgress

This value is assigned before single FRU VPD collection starts for the given FRU.

com.ibm.VPD.Collection.Status.NotStarted

This value is assigned when we hit prime inventory path when the VPD collection is not started as the FRU itself is not present.

Test:

busctl introspect xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
com.ibm.VPD.Collection interface - - -
.CollectionStatus property s "com.ibm.VPD.Collection.Status.Success" emits-change writable
busctl introspect xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/dimm0
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
com.ibm.VPD.Collection interface - - -
.CollectionStatus property s "com.ibm.VPD.Collection.Status.NotSta... emits-change writable
busctl introspect xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
com.ibm.VPD.Collection interface - - -
.CollectionStatus property s "com.ibm.VPD.Collection.Status.Failure" emits-change writable
Previous state is Failure && perform singleFRUVPDCollect busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Failure"
root@testhostname2:~busctl call com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectFRUVPD o /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson root@testhostname2: busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Failure"
Previous state is Success && perform singleFRUVPDCollect

busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/lcd_op_panel_hill com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Success"
root@testhostname2:busctl call com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectFRUVPD o /xyz/openbmc_project/inventory/system/chassis/motherboard/lcd_op_panel_hill

root@testhostname2:busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/lcd_op_panel_hill com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Success"

Previous state is NotStarted && perform singleFRUVPDCollect busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/dimm0 com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.NotStarted"

root@testhostname2:~ busctl call com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectFRUVPD o /xyz/openbmc_project/inventory/system/chassis/motherboard/dimm0 root@testhostname2:~ busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/dimm0 com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.NotStarted"

Change-Id: Ia5010a181f720454bb51538d6fcf308daf6b75ca

SunnySrivastava1984

This value is assigned when we hit prime inventory path on
VPD collection failure due to VPD exceptions and also when
single FRU VPD collection fails.

This is not correct.

SunnySrivastava1984 · 2024-12-18T13:45:54Z

vpd-manager/src/worker.cpp

-             logging::logMessage("Priming of inventory failed for FRU " +
-                                 i_vpdFilePath);
-         }*/
+        if (!primeInventory(i_vpdFilePath))


Why this has been un-commented? This breaks the current flow.

reverted back

SunnySrivastava1984 · 2024-12-18T13:47:13Z

vpd-manager/src/worker.cpp

        if (std::filesystem::exists(i_vpdFilePath))
        {
            l_propertyValueMap["Present"] = true;
+
+            // FRU exist and if we hit prime inventory path
+            processFRUCollectionStatus(l_interfaces,


I don't agree with this.
If execution is here, does not mean that the collection has failed.

SunnySrivastava1984 · 2024-12-18T14:05:42Z

vpd-manager/src/worker.cpp

@@ -1138,6 +1149,10 @@ void Worker::populateDbus(const types::VPDMapVariant& parsedVpdMap,
            processFunctionalProperty(inventoryPath, interfaces);
            processEnabledProperty(inventoryPath, interfaces);

+            // FRU VPD collection is successful
+            processFRUCollectionStatus(interfaces,


Where do we set the property to "InProgress" in worker class during collection of all the FRUs?

InProgress will not be displayed during collection of all FRUs as its an intermediate state between success/failure. InProgress was introduced for collecting single FRU only.

SunnySrivastava1984 · 2024-12-18T14:10:54Z

vpd-manager/src/worker.cpp

@@ -1138,6 +1149,10 @@ void Worker::populateDbus(const types::VPDMapVariant& parsedVpdMap,
            processFunctionalProperty(inventoryPath, interfaces);
            processEnabledProperty(inventoryPath, interfaces);

+            // FRU VPD collection is successful
+            processFRUCollectionStatus(interfaces,
+                                       constants::vpdCollectionSuccess);


Also we need to see where can we set it as failure.

SunnySrivastava1984 · 2024-12-18T14:14:32Z

vpd-manager/src/manager.cpp

@@ -447,6 +448,16 @@ void Manager::collectSingleFruVpd(
            }
        }

+        // Set CollectionStatus as InProgress before performing single FRU VPD
+        // collection
+        if (!dbusUtility::setFruVpdCollectionStatus(


I dont see a need to persist in-progress status using PIM. It is an intermediate state, no point persisting that data.
Notify is an expensive call compared to set property. If there is no requirement to persist the data, should we use set-property?

SunnySrivastava1984 · 2024-12-18T14:18:26Z

vpd-manager/src/worker.cpp

+{
+    types::PropertyMap l_fruCollectionProperty = {
+        {"CollectionStatus", i_fruCollectionStatus}};
+    vpdSpecificUtility::insertOrMerge(io_interfaces,


We already have "insertOrMerge" API in utility. Why this API is required? Just a value is being initialized in the API before "insertOrMerge" is called which could have been directly passed to the API.
Looks more like a wrapper over utility methods with no requirement.

removed this wrapper.

PriyangaRamasamy · 2024-12-19T11:09:04Z

Will address all the comments. currently keeping it as draft to avoid further reviews.

SunnySrivastava1984 · 2025-01-17T06:04:50Z

vpd-manager/src/worker.cpp

@@ -1103,6 +1111,7 @@ void Worker::populateDbus(const types::VPDMapVariant& parsedVpdMap,
        {
            const auto& inventoryPath = aFru["inventoryPath"];
            sdbusplus::message::object_path fruObjectPath(inventoryPath);
+


Why this change?

will remove it

SunnySrivastava1984 · 2025-01-17T06:08:19Z

vpd-manager/src/worker.cpp

@@ -1469,14 +1506,32 @@ std::tuple<bool, std::string>
            logging::logMessage(ex.what());
        }

+        // update CollectionStatus as Failure
+        types::ObjectMap l_objectMap;


Move this to a utility method so that manager can reuse it.

SunnySrivastava1984 · 2025-01-17T06:10:03Z

vpd-manager/src/worker.cpp

@@ -1375,7 +1392,6 @@ types::VPDMapVariant Worker::parseVpdFile(const std::string& i_vpdFilePath)

        std::shared_ptr<Parser> vpdParser =
            std::make_shared<Parser>(i_vpdFilePath, m_parsedJson);
-


This change?

will remove it

SunnySrivastava1984 · 2025-01-17T06:46:51Z

vpd-manager/src/worker.cpp

+        // D-bus set-property call is good enough to update the status.
+        try
+        {
+            const std::string& l_collStatusProp = "CollectionStatus";


Why a variable is needed here? Its a one time use variable. Can be used in place?

dbusUtility::writeDbusProperty() expects ref of the property variable. so I need a memory address to be allocated

That's ok, it anyways accepts constant reference.

SunnySrivastava1984 · 2025-01-17T06:54:49Z

vpd-manager/src/worker.cpp

@@ -1421,6 +1437,9 @@ types::VPDMapVariant Worker::parseVpdFile(const std::string& i_vpdFilePath)
 std::tuple<bool, std::string>
    Worker::parseAndPublishVPD(const std::string& i_vpdFilePath)
 {
+    const std::string& l_inventoryPath =
+        jsonUtility::getInventoryObjPathFromJson(m_parsedJson, i_vpdFilePath);


This API can throw exceptions and this can go uncaught. This will break the threading for all other collections.

Let the variable be at outer scope, bring the API call to "getInventoryObjPathFromJson" under try/catch.

SunnySrivastava1984 · 2025-01-21T04:19:49Z

vpd-manager/src/worker.cpp

+        // D-bus set-property call is good enough to update the status.
+        try
+        {
+            const std::string& l_collStatusProp = "CollectionStatus";


That's ok, it anyways accepts constant reference.

SunnySrivastava1984 · 2025-01-21T04:24:38Z

vpd-manager/include/utility/vpd_specific_utility.hpp

+ *
+ * @return true if update succeeds, false otherwise.
+ */
+inline bool notifyFRUCollectionStatus(const std::string& i_inventoryPath,


This API just updates a property on DBus.
Should this be under dbusUtility?

moved it to dbusUtility

This commit implements CollectionStatus D-bus property under com.ibm.VPD.Collection D-bus interface for each inventory D-bus object path which represents a FRU. The property tells the current status of VPD collection for a given FRU's D-bus object path. The property takes the below enum values: >>>com.ibm.VPD.Collection.Status.Success ------------------------------------- This value is assigned when VPD collection is successful. >>>com.ibm.VPD.Collection.Status.Failure ------------------------------------- VPD collection failure due to VPD exceptions. >>>com.ibm.VPD.Collection.Status.InProgress ---------------------------------------- This value is assigned when VPD collection starts for the given FRU. >>>com.ibm.VPD.Collection.Status.NotStarted ---------------------------------------- This default value is assigned when we hit prime inventory path. Test: 1. VPD parsing failed for /sys/bus/i2c/drivers/at24/0-0051/eeprom due to error: Unable to determine VPD format busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.NotStarted" busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.InProgress" busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/tpm_wilson com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Failure" 2. FRU not found busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/pcieslot0/pcie_card0 com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.NotStarted" busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/pcieslot0/pcie_card0 com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.InProgress" busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard/pcieslot0/pcie_card0 com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Failure" 3. Successful collection of VPD busctl get-property xyz.openbmc_project.Inventory.Manager /xyz/openbmc_project/inventory/system/chassis/motherboard com.ibm.VPD.Collection CollectionStatus s "com.ibm.VPD.Collection.Status.Success" Change-Id: Ia5010a181f720454bb51538d6fcf308daf6b75ca Signed-off-by: Priyanga Ramasamy <[email protected]>

PriyangaRamasamy force-pushed the collectedDbusProperty branch from a39536e to 24cdfd2 Compare December 18, 2024 12:23

PriyangaRamasamy requested review from SunnySrivastava1984, branupama, jinuthomas, souvik1914581 and RekhaAparna01 December 18, 2024 12:24

SunnySrivastava1984 requested changes Dec 19, 2024

View reviewed changes

PriyangaRamasamy marked this pull request as draft December 19, 2024 11:09

PriyangaRamasamy force-pushed the collectedDbusProperty branch from 24cdfd2 to 274baa5 Compare January 9, 2025 17:49

PriyangaRamasamy marked this pull request as ready for review January 9, 2025 17:50

PriyangaRamasamy changed the title ~~Implement CollectionStatus for each inventory FRU~~ worker::CollectionStatus for each inventory FRU Jan 9, 2025

PriyangaRamasamy force-pushed the collectedDbusProperty branch from 274baa5 to 4a152f1 Compare January 13, 2025 12:30

SunnySrivastava1984 requested changes Jan 17, 2025

View reviewed changes

PriyangaRamasamy force-pushed the collectedDbusProperty branch 2 times, most recently from 8040efd to 6956b31 Compare January 17, 2025 10:40

SunnySrivastava1984 requested changes Jan 21, 2025

View reviewed changes

PriyangaRamasamy force-pushed the collectedDbusProperty branch from 6956b31 to 5ff3795 Compare January 21, 2025 10:03

PriyangaRamasamy requested a review from SunnySrivastava1984 January 21, 2025 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker::CollectionStatus for each inventory FRU #547

worker::CollectionStatus for each inventory FRU #547

PriyangaRamasamy commented Dec 18, 2024 •

edited

Loading

SunnySrivastava1984 left a comment

SunnySrivastava1984 Dec 18, 2024

PriyangaRamasamy Jan 3, 2025

SunnySrivastava1984 Dec 18, 2024

SunnySrivastava1984 Dec 18, 2024

PriyangaRamasamy Jan 3, 2025

SunnySrivastava1984 Dec 18, 2024

SunnySrivastava1984 Dec 18, 2024

PriyangaRamasamy Jan 3, 2025

SunnySrivastava1984 Dec 18, 2024

PriyangaRamasamy Jan 3, 2025

PriyangaRamasamy commented Dec 19, 2024

SunnySrivastava1984 Jan 17, 2025

PriyangaRamasamy Jan 17, 2025

SunnySrivastava1984 Jan 17, 2025

PriyangaRamasamy Jan 17, 2025

SunnySrivastava1984 Jan 17, 2025

PriyangaRamasamy Jan 17, 2025

SunnySrivastava1984 Jan 17, 2025

PriyangaRamasamy Jan 17, 2025

SunnySrivastava1984 Jan 21, 2025

PriyangaRamasamy Jan 21, 2025

SunnySrivastava1984 Jan 17, 2025

PriyangaRamasamy Jan 17, 2025

SunnySrivastava1984 Jan 21, 2025

SunnySrivastava1984 Jan 21, 2025

PriyangaRamasamy Jan 21, 2025

		@@ -1375,7 +1392,6 @@ types::VPDMapVariant Worker::parseVpdFile(const std::string& i_vpdFilePath)

		std::shared_ptr<Parser> vpdParser =
		std::make_shared<Parser>(i_vpdFilePath, m_parsedJson);

worker::CollectionStatus for each inventory FRU #547

Are you sure you want to change the base?

worker::CollectionStatus for each inventory FRU #547

Conversation

PriyangaRamasamy commented Dec 18, 2024 • edited Loading

SunnySrivastava1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PriyangaRamasamy commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PriyangaRamasamy commented Dec 18, 2024 •

edited

Loading