Topic/rdkb 63981#107
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the rfc.service systemd unit to add timestamped logging around the pre-start delay and RFC service startup, improving observability during boot/service bring-up.
Changes:
- Add START/END timestamp logging to
ExecStartPrearound the existing 300s sleep. - Add timestamped logging when appending “Enable RFC feature” to
${PARODUS_START_LOG_FILE}. - Add START/END timestamp logging around spawning
/lib/rdk/rfc.service.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExecStart=/bin/sh -c 'echo "Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' | ||
| ExecStart=/bin/sh -c '/lib/rdk/rfc.service &' | ||
| ExecStart=/bin/sh -c 'echo " $(date) Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' | ||
| ExecStart=/bin/sh -c 'echo "RFC execStart START: $(date)"; sleep 60; echo "RFC execStart END: $(date)"' |
There was a problem hiding this comment.
This ExecStart command will always exit with the status of the final echo, which effectively masks failures starting /lib/rdk/rfc.service (e.g., exec permission/path issues). Also, the "END" log is emitted immediately after spawning the background job, which can be misleading. Consider preserving/propagating the start command’s exit status (or removing the trailing echo), and rename the message to reflect that the process was spawned rather than completed if it remains backgrounded.
| ExecStart=/bin/sh -c 'echo "RFC execStart START: $(date)"; sleep 60; echo "RFC execStart END: $(date)"' | |
| ExecStart=/bin/sh -ec 'echo "RFC execStart START: $(date)"; sleep 60; echo "RFC execStart END: $(date)"' |
| EnvironmentFile=/etc/device.properties | ||
| ExecStart=/bin/sh -c 'echo "Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' | ||
| ExecStart=/bin/sh -c '/lib/rdk/rfc.service &' | ||
| ExecStart=/bin/sh -c 'echo " $(date) Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' |
There was a problem hiding this comment.
The log line being appended includes a leading space before the timestamp (" $(date) ..."), which makes log parsing/grepping inconsistent and looks accidental. Trim the leading space (and consider matching the repository’s common date: log format used in other unit scripts).
| ExecStart=/bin/sh -c 'echo " $(date) Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' | |
| ExecStart=/bin/sh -c 'echo "$(date) Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Service] | ||
| Type=oneshot | ||
| ExecStartPre=/bin/sh -c 'sleep 300' | ||
| ExecStartPre=/bin/sh -c 'echo "RFC execPre START: $(date)"; sleep 300; echo "RFC execPre END: $(date)"' |
There was a problem hiding this comment.
ExecStartPre is doing three actions (log, sleep, log) via a shell -c string. For systemd units this is harder to reason about and makes it difficult to adjust timeouts/ordering later. Consider splitting this into multiple ExecStartPre= directives (systemd runs them sequentially) and/or using native commands (e.g., ExecStartPre=/bin/sleep 300) so each step is separately visible and failure behavior is clearer.
| ExecStartPre=/bin/sh -c 'echo "RFC execPre START: $(date)"; sleep 300; echo "RFC execPre END: $(date)"' | |
| ExecStartPre=/bin/sh -c 'echo "RFC execPre START: $(date)"' | |
| ExecStartPre=/bin/sleep 300 | |
| ExecStartPre=/bin/sh -c 'echo "RFC execPre END: $(date)"' |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ExecStart=/bin/sh -c '/lib/rdk/rfc.service &' | ||
| ExecStart=/bin/sh -c 'echo " $(date) Enable RFC feature" >> ${PARODUS_START_LOG_FILE}' | ||
| ExecStart=/bin/sh -c 'echo "SYSTEMD CALL $(date)" >> /rdklogs/logs/rfc_trace.log' | ||
| ExecStart=/bin/sh -c 'echo "RFC execStart START: $(date)"; /lib/rdk/rfc.service &' |
There was a problem hiding this comment.
Running the main service script in the background ('&') under Type=oneshot means systemd won’t reliably track the long-running process, and the unit can report success even if /lib/rdk/rfc.service fails immediately. Prefer letting systemd manage the process directly (e.g., Type=simple with ExecStart=/lib/rdk/rfc.service, or Type=forking with a PIDFile if it daemonizes) so restarts/status reflect the real process state.
No description provided.