Create Notify service framework #69
Conversation
aryakpadman
force-pushed
the
notify_service_framework
branch
from
25e6316 to
c7b7d65
Compare
aryakpadman
requested review from
RameshIyyar,
harsh-agarwal1,
manishtwari and
riya461
and removed request for
harsh-agarwal1
- This change adds a framework to handle notification requests received
from a sibling BMC and trigger the appropriate actions.
- The notification requests are copied from the sibling BMC to
`/var/lib/phosphor-data-sync/services/`.
- The framework parses these JSON files and triggers actions based on
the `Mode` and `Method` fields in each request.
- If `Mode` is `Systemd` and `Method` is `Restart`, a systemd
restart is initiated for all applications listed in the
`NotifyServices` field.
- If `Method` is `Reload`, a systemd reload is triggered instead.
- In both cases, ModifiedPath is not passed to the applications
since systemd does not currently support this option.
Change-Id: I0efdf7f774a4dedf06d5e3ce0642673cf5f5da5d
Signed-off-by: Arya K Padman <[email protected]>
aryakpadman
force-pushed
the
notify_service_framework
branch
from
c7b7d65 to
a6cde0f
Compare
- This change introduces framework APIs to initiate D-Bus notification calls when the configured notification Mode is set to DBus. - Applications that need to receive notifications are expected to implement the corresponding D-Bus interface and method. - The full implementation is not included in this change, as the PDI interface and methods are still under discussion. Change-Id: I574a297bc850ef4fd797ae8b5f4c1533fc19bbc5 Signed-off-by: Arya K Padman <[email protected]>
aryakpadman
force-pushed
the
notify_service_framework
branch
from
a6cde0f to
6c6c065
Compare
| 'external_data_ifaces.cpp', | ||
| 'external_data_ifaces_impl.cpp', | ||
| 'manager.cpp', | ||
| 'notify_service.cpp', |
There was a problem hiding this comment.
Commit message:
- The notification requests are copied from the sibling BMC to
/var/lib/phosphor-data-sync/services/.
path is correct? I think /var/lib/phosphor-data-sync/notify-services/?
| } | ||
| else | ||
| { | ||
| lg2::error("Failed to process the notify request[{PATH}], Error : " |
There was a problem hiding this comment.
Please include the file path we are notifying and include mode value as well. It will help during debugging to identify which file failed to notify.
|
|
||
| sdbusplus::async::task<> | ||
| // NOLINTNEXTLINE | ||
| NotifyService::sendSystemDNotification( |
There was a problem hiding this comment.
Is it possible to move this into the external interface class? I’d prefer to keep all DBus calls consolidated
| // NOLINTNEXTLINE | ||
| co_await sendSystemDNotification( | ||
| ctx, | ||
| notifyfileData["NotifyInfo"]["NotifyServices"] |
There was a problem hiding this comment.
Better to pass the JSON object as a const reference and use the fields as needed. For example, we can include the file-path details in the trace if a service fails to reload or restart.
| catch (const sdbusplus::exception::SdBusError& e) | ||
| { | ||
| lg2::error( | ||
| "Failed to send systemd notification request to {SERVICE}: {ERROR}", |
There was a problem hiding this comment.
Good to have filepath details , I feel we could able to get from the NofifyInfo data.
| try | ||
| { | ||
| using objectPath = sdbusplus::message::object_path; | ||
| if (method == "Restart") |
There was a problem hiding this comment.
You could merge this if and else if block.
dbusMethodName = (method == "Reload" ? "ReloadUnit" : "RestartUnit");
// Added info trace so that we will able to know why some service restarted/reloaded otherwise we wont able to know why it is restarted/reloaded.
lg2::info("{METHOD} {SERVICE} to notify synced data", "METHOD", dbusMethodName, "SERVICE", service);
co_await systemdReload.call<objectPath>(ctx, dbusMethodName, service, "replace");
| { | ||
| std::ifstream notifyFile; | ||
| notifyFile.open(fs::path(NOTIFY_SERVICES_DIR) / notifyFilePath); | ||
| nlohmann::json notifyInfoJSON(nlohmann::json::parse(notifyFile)); |
There was a problem hiding this comment.
22 and 24 could be combined.
return nlohmann::json(nlohmann::json::parse(notifyFile) - If parse return the JSON then return nlohmann::json::parse(notifyFile);
| ctx.spawn(init(ctx, notifyFilePath)); | ||
| } | ||
|
|
||
| sdbusplus::async::task<std::string> |
There was a problem hiding this comment.
Lets hold this commit?
There was a problem hiding this comment.
May be we can, Can push it when DBus interface things finalized.
3 participants
No description provided.