RDKB-63950 : Chrony Integration in RDKB - #390
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Chrony (chronyd) as an alternative NTP client in RDK-B while keeping ntpd as the default, with an RFC-controlled runtime switch via /nvram/chrony_enabled.
Changes:
- Adds an RFC-flag guard to skip starting
ntpdwhen Chrony is enabled. - Introduces a new
service_chronyd.shhandler to start/stop Chrony and update NTP sync state. - Registers the new
chronydservice in the init c_registration build and adds a new10_chronydregistrar binary.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/scripts/init/service.d/service_ntpd.sh | Adds RFC guard to bypass ntpd startup when Chrony is enabled. |
| source/scripts/init/service.d/service_chronyd.sh | New Chrony service handler with RFC gating, WAN/connectivity checks, and sync-status monitoring. |
| source/scripts/init/c_registration/Makefile.am | Builds/installs new 10_chronyd registrar program. |
| source/scripts/init/c_registration/10_chronyd.c | Registers chronyd with sysevent, mirroring ntpd-style lifecycle integration. |
Comments suppressed due to low confidence (2)
source/scripts/init/service.d/service_chronyd.sh:186
- In
service_start,rcis taken from$?after theif pidof ntpd ...block. When ntpd is not running,pidofreturns non-zero andrcbecomes non-zero, causing an early error return and preventing chronyd from starting. Even when ntpd is running,rcends up reflectingsleeprather thansystemctl stop.
# Stop ntpd if running — mutual exclusivity with chrony
if pidof ntpd > /dev/null 2>&1; then
echo_t "SERVICE_CHRONYD : stopping ntpd for mutual exclusivity" >> $NTPD_LOG_NAME
systemctl stop ntpd
killall ntpd 2>/dev/null
sleep 2
fi
local rc=$?
if [ "$rc" -ne 0 ]; then
sysevent set ${SERVICE_NAME}-status "error"
return 1
fi
source/scripts/init/service.d/service_chronyd.sh:140
service_startlogs that it is waiting for the connectivity check file but the actual wait call is commented out, so chronyd may proceed before connectivity check completes (and the logs are misleading).
# Wait for connectivitycheck to complete
if [ -f $CONNCHECK_FILE ]; then
echo_t "SERVICE_NTPD CONNCHK: connectivity success $CONNCHECK_FILE present" >> $NTPD_LOG_NAME
else
# Exclude XLE device from connectivity check. TODO
if [ "$BOX_TYPE" != "WNXL11BWL" ];then
echo_t "SERVICE_NTPD CONNCHK: start connectivity check waiting for $CONNCHECK_FILE file" >> $NTPD_LOG_NAME
#waitForConnChkFile
fi
fi
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
source/scripts/init/service.d/service_ntpd.sh:339
- When the chrony RFC flag is present, this returns early without updating the ntpd service status. That can leave
${SERVICE_NAME}-statusstale (e.g., still "started" from a prior run) even though ntpd is intentionally inactive.
if [ -f "$RFC_FLAG" ]; then
echo_t "SERVICE_NTPD : RFC flag Present — chrony is the active NTP client" >> $NTPD_LOG_NAME
sysevent set ${SERVICE_NAME}-status "stopped"
if pidof "$BIN" > /dev/null 2>&1; then
source/scripts/init/service.d/service_chronyd.sh:114
- This log line uses the
SERVICE_NTPDprefix in the chronyd script, which is misleading when diagnosing connectivity-check timeouts.
echo_t "SERVICE_NTPD CONNCHK: Timeout ${TIMEOUT}s expired - file $CONNCHECK_FILE not found" >> $NTPD_LOG_NAME
source/scripts/init/service.d/service_chronyd.sh:133
- This connectivity-check success log line uses the
SERVICE_NTPDprefix in the chronyd service, which makes the logs ambiguous.
echo_t "SERVICE_NTPD CONNCHK: connectivity success $CONNCHECK_FILE present" >> $NTPD_LOG_NAME
source/scripts/init/service.d/service_chronyd.sh:137
- This connectivity-check wait log line uses the
SERVICE_NTPDprefix in the chronyd service, which makes the logs ambiguous.
echo_t "SERVICE_NTPD CONNCHK: start connectivity check waiting for $CONNCHECK_FILE file" >> $NTPD_LOG_NAME
source/scripts/init/service.d/service_chronyd.sh:270
- The registration in
10_chronyd.cincludesipv6_connection_stateon HUB4 builds, but this script has no matchingcasebranch. That event will fall into the default path and exit with a usage error instead of attempting a start.
wan-status)
if [ "started" = "$CURRENT_WAN_STATUS" ]; then
# First sync this boot — service_start() guards against duplicate instances via pidof
echo_t "SERVICE_CHRONYD : wan-status=started, calling service_start" >> $NTPD_LOG_NAME
service_start
source/scripts/init/service.d/service_chronyd.sh:139
- The script logs that it is "waiting" for the connectivity-check file, but the actual wait call is commented out, so chronyd will proceed immediately. Either call
waitForConnChkFile(to match ntpd behavior) or adjust the log/message accordingly.
if [ "$BOX_TYPE" != "WNXL11BWL" ];then
echo_t "SERVICE_NTPD CONNCHK: start connectivity check waiting for $CONNCHECK_FILE file" >> $NTPD_LOG_NAME
#waitForConnChkFile
fi
source/scripts/init/service.d/service_chronyd.sh:187
local rc=$?here does not reflect whether stopping ntpd succeeded (it ends up taking the status of the last command in theifblock, e.g.,sleep 2, or 0 when theifis skipped). This makes the error-check misleading and effectively dead code.
local rc=$?
if [ "$rc" -ne 0 ]; then
sysevent set ${SERVICE_NAME}-status "error"
return 1
fi
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
source/scripts/init/service.d/service_chronyd.sh:115
- Log prefix is
SERVICE_NTPDin the chronyd connectivity-check timeout message, which makes logs misleading and harder to grep/parse for chronyd behavior.
if [ "$ELAPSED" -ge "$TIMEOUT" ]; then
echo_t "SERVICE_NTPD CONNCHK: Timeout ${TIMEOUT}s expired - file $CONNCHECK_FILE not found" >> $NTPD_LOG_NAME
return 1
source/scripts/init/service.d/service_chronyd.sh:140
- This branch logs that it is "waiting" for the connectivity-check file but the actual wait call is commented out, and the log prefix says
SERVICE_NTPD. Either wait for the file (like service_ntpd.sh does) or adjust the log so it matches behavior; otherwise chronyd can start before connectivity is confirmed and logs are misleading.
if [ -f $CONNCHECK_FILE ]; then
echo_t "SERVICE_NTPD CONNCHK: connectivity success $CONNCHECK_FILE present" >> $NTPD_LOG_NAME
else
# Exclude XLE device from connectivity check. TODO
if [ "$BOX_TYPE" != "WNXL11BWL" ];then
echo_t "SERVICE_NTPD CONNCHK: start connectivity check waiting for $CONNCHECK_FILE file" >> $NTPD_LOG_NAME
#waitForConnChkFile
fi
fi
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
source/scripts/init/service.d/service_chronyd.sh:140
- The connectivity-check gate in service_start() logs that it is waiting, but the actual wait call is commented out. This means chronyd may start before connectivitycheck completes, unlike the ntpd path (service_ntpd.sh calls waitForConnChkFile). Also, the log tag here says SERVICE_NTPD, which is misleading in the chronyd service.
# Wait for connectivitycheck to complete
if [ -f $CONNCHECK_FILE ]; then
echo_t "SERVICE_NTPD CONNCHK: connectivity success $CONNCHECK_FILE present" >> $NTPD_LOG_NAME
else
# Exclude XLE device from connectivity check. TODO
if [ "$BOX_TYPE" != "WNXL11BWL" ];then
echo_t "SERVICE_NTPD CONNCHK: start connectivity check waiting for $CONNCHECK_FILE file" >> $NTPD_LOG_NAME
#waitForConnChkFile
fi
fi
source/scripts/init/service.d/service_chronyd.sh:115
- This timeout log line is emitted by service_chronyd.sh but is tagged as SERVICE_NTPD, which makes troubleshooting confusing.
if [ "$ELAPSED" -ge "$TIMEOUT" ]; then
echo_t "SERVICE_NTPD CONNCHK: Timeout ${TIMEOUT}s expired - file $CONNCHECK_FILE not found" >> $NTPD_LOG_NAME
return 1
source/scripts/init/service.d/service_chronyd.sh:76
- set_chrony_sync_status() assigns to
leapwithout declaring it local, which can leak state into the outer script scope and make future edits fragile.
leap=$(chronyc tracking 2>/dev/null | grep "Leap status" | awk '{print $NF}')
if [ "$leap" = "Normal" ]; then
echo_t "SERVICE_CHRONYD : time sync confirmed (Leap status Normal)" >> $NTPD_LOG_NAME
source/scripts/init/service.d/service_chronyd.sh:152
- This block runs
pidoftwice (once for the check, once for logging), and the logged pid can also differ from the one that was checked. Capture the pid once and reuse it.
# Do not start a second instance if chronyd is already running
if pidof "$CHRONY_BIN" > /dev/null 2>&1; then
echo_t "SERVICE_CHRONYD : already running (pid=$(pidof $CHRONY_BIN)), skipping start" >> $NTPD_LOG_NAME
return 0
fi
source/scripts/init/service.d/service_ntpd.sh:338
- This new log message introduces a non-ASCII em dash character (—). Shell scripts/log pipelines on embedded targets are often assumed to be ASCII; using plain '-' avoids encoding issues in tooling and log parsers.
if [ -f "$RFC_FLAG" ]; then
echo_t "SERVICE_NTPD : RFC flag Present — chrony is the active NTP client" >> $NTPD_LOG_NAME
sysevent set ${SERVICE_NAME}-status "stopped"
source/scripts/init/c_registration/10_chronyd.c:31
- The comment says no compile-time flag is needed for ipv6_connection_state registration, but the code is actually gated on HUB4_PRODUCT_REQ. This is minor, but it makes the intent unclear for readers.
/*
* Listen to the same network events as ntpd so chronyd starts when WAN comes up.
* ipv6_connection_state is registered for HUB4/SKY platforms (guarded in the
* handler script itself — no compile-time flag needed here because the RFC flag
* /nvram/chrony_enabled gates all execution in service_chronyd.sh).
*/
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
source/scripts/init/service.d/service_chronyd.sh:49
LANIPV6SupportandWAN_INTERFACEare assigned but never used. Removing unused variables avoids confusion and avoids unnecessarysysevent/getWanInterfaceNamecalls.
LANIPV6Support=$(sysevent get LANIPv6GUASupport)
CURRENT_WAN_STATUS=$(sysevent get wan-status)
WAN_INTERFACE=$(getWanInterfaceName)
source/scripts/init/service.d/service_chronyd.sh:255
ipv6_connection_stateis listed in the usage string and is registered as a sysevent handler in10_chronyd.cunder_HUB4_PRODUCT_REQ_, but this script has noipv6_connection_state)case. On those builds the event will fall into*), print usage, and exit(3) instead of handling the event.
fi
;;
*)
echo "Usage: $SELF_NAME [ ${SERVICE_NAME}-start | ${SERVICE_NAME}-stop | ${SERVICE_NAME}-restart | wan-status | ipv6_connection_state ]" >&2
rm -f "$LOCKFILE"
source/scripts/init/service.d/service_chronyd.sh:129
- When the RFC flag is absent,
service_startreturns without updating${SERVICE_NAME}-statusand without stopping an already-runningchronyd. That can leave chronyd running when switching back to ntpd, which undermines the PR’s “runtime control to switch between ntpd and chronyd”.
# RFC guard — only run if flag is present
if [ ! -f "$RFC_FLAG" ]; then
echo_t "SERVICE_CHRONYD : RFC flag absent — chrony path inactive" >> $NTPD_LOG_NAME
return 0
fi
source/scripts/init/service.d/service_chronyd.sh:71
- The background sync monitor stops checking after 120s (
MAX_RETRY=12with 10s sleeps). If chrony takes longer than 2 minutes to reachLeap status Normal, this function exits andntp_time_sync/ntp_status=3are never set, even if time sync completes later.
local MAX_RETRY=12 # 12 × 10s = 120s max wait
while true; do
if [ "$retry" -gt "$MAX_RETRY" ]; then
echo_t "SERVICE_CHRONYD : sync not confirmed within 120s — daemon still running" >> $NTPD_LOG_NAME
source/scripts/init/service.d/service_chronyd.sh:30
CHRONY_CONF_TMPis declared but never used in this script, which makes the config handling look incomplete/misleading.
This issue also appears on line 47 of the same file.
CHRONY_CONF_TMP=/etc/rdk_chrony.conf
CHRONY_BIN=chronyd
Reason for change:
ntpdandchronydTest Procedure: Enable the chrony RFC and check if chrony is the active NTP Client
Risks: Medium
Priority: P1
Signed-off-by: Sindhuja Sindhuja_Muthukrishnan@comcast.com