Skip to content

RDKB-63950 : Chrony Integration in RDKB - #390

Open
sindhu-krishnan wants to merge 32 commits into
developfrom
topic/RDKB-63950
Open

RDKB-63950 : Chrony Integration in RDKB#390
sindhu-krishnan wants to merge 32 commits into
developfrom
topic/RDKB-63950

Conversation

@sindhu-krishnan

@sindhu-krishnan sindhu-krishnan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Reason for change:

  • Integrates Chrony as the alternative NTP client in RDK-B.
  • Retains ntpd as the default NTP client.
  • Introduces RFC runtime control to switch between ntpd and chronyd
    Test 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

Copilot AI review requested due to automatic review settings July 28, 2026 05:41
@sindhu-krishnan
sindhu-krishnan requested review from a team as code owners July 28, 2026 05:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ntpd when Chrony is enabled.
  • Introduces a new service_chronyd.sh handler to start/stop Chrony and update NTP sync state.
  • Registers the new chronyd service in the init c_registration build and adds a new 10_chronyd registrar 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, rc is taken from $? after the if pidof ntpd ... block. When ntpd is not running, pidof returns non-zero and rc becomes non-zero, causing an early error return and preventing chronyd from starting. Even when ntpd is running, rc ends up reflecting sleep rather than systemctl 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_start logs 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.

Comment thread source/scripts/init/service.d/service_chronyd.sh Outdated
Comment thread source/scripts/init/service.d/service_ntpd.sh
Comment thread source/scripts/init/c_registration/10_chronyd.c
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 28, 2026 05:54
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}-status stale (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_NTPD prefix 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_NTPD prefix 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_NTPD prefix 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.c includes ipv6_connection_state on HUB4 builds, but this script has no matching case branch. 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 the if block, e.g., sleep 2, or 0 when the if is 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

Copilot AI review requested due to automatic review settings July 28, 2026 05:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_NTPD in 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

Comment thread source/scripts/init/service.d/service_chronyd.sh Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 28, 2026 07:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 leap without 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 pidof twice (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).
 */

Copilot AI review requested due to automatic review settings July 29, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • LANIPV6Support and WAN_INTERFACE are assigned but never used. Removing unused variables avoids confusion and avoids unnecessary sysevent/getWanInterfaceName calls.
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_state is listed in the usage string and is registered as a sysevent handler in 10_chronyd.c under _HUB4_PRODUCT_REQ_, but this script has no ipv6_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_start returns without updating ${SERVICE_NAME}-status and without stopping an already-running chronyd. 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=12 with 10s sleeps). If chrony takes longer than 2 minutes to reach Leap status Normal, this function exits and ntp_time_sync/ntp_status=3 are 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_TMP is 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants