Conversation
There was a problem hiding this comment.
Pull request overview
Integrates chronyd into the Yocto build to provide time synchronization, including default chrony configuration, a systemd service unit, and a volatile bind for runtime-updated server settings.
Changes:
- Added default
/etc/chrony.confand a runtime-updated include file (/etc/rdk_chrony.conf). - Added a
chronyd.servicesystemd unit to start chronyd. - Updated distro config to bind-mount the runtime chrony server config from a volatile path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| recipes-support/chrony/files/rdk_chrony.conf | Adds RDK-specific NTP server config intended to be updated dynamically. |
| recipes-support/chrony/files/chronyd.service | Introduces a systemd unit to run chronyd at boot. |
| recipes-support/chrony/files/chrony.conf | Provides base chrony defaults and includes the dynamic RDK config file. |
| recipes-support/chrony/chrony_%.bbappend | Installs the new config files and service unit into the image/package. |
| conf/distro/rdk.conf | Adds a volatile bind mount for the dynamic chrony server configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logchange 0.5 | ||
|
|
||
| #Logging | ||
| logdir /opt/logs/chrony |
There was a problem hiding this comment.
The config sets logdir /opt/logs/chrony, but neither the recipe nor the service ensures this directory exists. If the directory is missing at boot, chronyd logging can fail (and may prevent startup depending on chronyd settings). Create /opt/logs/chrony during install (or via tmpfiles.d) and/or in ExecStartPre.
| logdir /opt/logs/chrony | |
| logdir /var/log/chrony |
| rm ${D}${sysconfdir}/chrony.conf | ||
| install -m 755 ${WORKDIR}/chrony.conf ${D}${sysconfdir}/ | ||
| install -m 755 ${WORKDIR}/rdk_chrony.conf ${D}${sysconfdir}/ | ||
|
|
||
| # service to start chrony | ||
| rm ${D}${systemd_unitdir}/system/chronyd.service |
There was a problem hiding this comment.
The rm commands in do_install:append() are not using -f. If the upstream recipe changes paths or the files are not present for a given build configuration, the build will fail here. Use a non-fatal removal (e.g., rm -f) or guard with a file-exists check.
| rm ${D}${sysconfdir}/chrony.conf | |
| install -m 755 ${WORKDIR}/chrony.conf ${D}${sysconfdir}/ | |
| install -m 755 ${WORKDIR}/rdk_chrony.conf ${D}${sysconfdir}/ | |
| # service to start chrony | |
| rm ${D}${systemd_unitdir}/system/chronyd.service | |
| rm -f ${D}${sysconfdir}/chrony.conf | |
| install -m 755 ${WORKDIR}/chrony.conf ${D}${sysconfdir}/ | |
| install -m 755 ${WORKDIR}/rdk_chrony.conf ${D}${sysconfdir}/ | |
| # service to start chrony | |
| rm -f ${D}${systemd_unitdir}/system/chronyd.service |
vbabypublic
left a comment
There was a problem hiding this comment.
Hope the below settings in chronyd.service is a temporary solution for testing
Conflicts=systemd-timesyncd.service
We should have a proper solution for mutually exclusive use of chronyd and timesyncd
Could you fill below ticket with the details for review with OSS governance team?
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CONFFILES:${PN} += "${sysconfdir}/chrony.conf" | ||
| CONFFILES:${PN} += "${sysconfdir}/rdk_chrony.conf" |
There was a problem hiding this comment.
CONFFILES points at ${sysconfdir}/default/chrony.conf, but this bbappend installs the config to ${sysconfdir}/chrony.conf. This means the installed chrony.conf won’t be treated as a conffile (and the referenced conffile path likely doesn’t exist), which can lead to config being overwritten on upgrade. Update CONFFILES to match the actual installed path (and include ${sysconfdir}/chrony.conf).
| CONFFILES:${PN} += "${sysconfdir}/chrony.conf" | |
| CONFFILES:${PN} += "${sysconfdir}/rdk_chrony.conf" | |
| CONFFILES:${PN} = "${sysconfdir}/chrony.conf \ | |
| ${sysconfdir}/rdk_chrony.conf" |
| Type=forking | ||
| PIDFile=/run/chrony/chronyd.pid | ||
| ExecStartPre=/bin/mkdir -p /var/lib/chrony | ||
| ExecStart=/usr/sbin/chronyd |
There was a problem hiding this comment.
ExecStart=/usr/sbin/chronyd has trailing whitespace. This can cause noisy diffs and occasionally trips linters; remove the extra spaces.
| ExecStart=/usr/sbin/chronyd | |
| ExecStart=/usr/sbin/chronyd |
| @@ -0,0 +1,38 @@ | |||
| FILESEXTRAPATHS:prepend := "${THISDIR}/files:" | |||
There was a problem hiding this comment.
does this change affect the dependent module like systime manager ?
There was a problem hiding this comment.
This NTP client change does not impact SystimeMgr. Deep sleep will be handled in Systimemgr in later changes. There are no other dependencies.
| FILESEXTRAPATHS:prepend := "${THISDIR}/files:" | ||
|
|
||
|
|
||
| SRC_URI += "file://chrony.conf \ |
There was a problem hiding this comment.
are the any service in RDKE stack that depends on systemd-timesyncd ?
| install -m 0644 ${WORKDIR}/chronyd.service ${D}${systemd_unitdir}/system/ | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
These files should be moved to oss repo
|
Hi Deva, Sindhuja, As the current PR rdkcentral/meta-rdk#561 meant as a temporary solution, can we discuss/decide on following ? (1) The next OSS release 4.11.0 is planned on Friday, February 27. Do you need to move the bbappend in meta-rdk to meta-rdk-oss-reference and include in this release? If so, could you create a PR for that ? (2) Please find the review comments in the PR. The content of bbappend cannot be fully move to meta-rdk-oss-reference due to sensitive URLs (server time.xfinity.com). Even it cannot be kept in meta-rdk since it is a public repository |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Unit] | ||
| Description=NTP client/server | ||
| Documentation=man:chronyd(8) man:chrony.conf(5) | ||
| After=var-lib.mount nvram.service |
There was a problem hiding this comment.
The service is set to WantedBy=network-up.target but doesn't have any explicit network dependency in the After= directive (e.g., network-online.target or network.target). This could cause chronyd to start before the network is ready, leading to initial NTP sync failures. Consider adding "After=network-online.target" or a similar network dependency to ensure the network is available before chronyd starts.
| After=var-lib.mount nvram.service | |
| After=network-online.target var-lib.mount nvram.service |
| install -m 0755 ${S}/chronyc ${D}${sbindir} | ||
|
|
||
| #config File | ||
| rm ${D}${sysconfdir}/chrony.conf |
There was a problem hiding this comment.
This line has trailing whitespace after the command. Remove the trailing space to maintain code cleanliness.
| rm ${D}${sysconfdir}/chrony.conf | |
| rm ${D}${sysconfdir}/chrony.conf |
| # Binaries | ||
| install -m 0755 ${S}/chronyc ${D}${sbindir} | ||
|
|
||
| #config File |
There was a problem hiding this comment.
Comment formatting is inconsistent. Line 10 uses "# Binaries" (with space after #), while this line uses "#config File" (no space after #). For consistency, use a space after # as seen in other comments in this file.
| #config File | |
| # Config File |
| do_install:append() { | ||
| # Binaries | ||
| install -m 0755 ${S}/chronyc ${D}${sbindir} |
There was a problem hiding this comment.
The chrony.conf specifies "logdir /opt/logs/chrony" but the do_install function does not create this directory. This may cause chronyd to fail at runtime if the directory doesn't exist. Consider adding a command like "install -d ${D}/opt/logs/chrony" in the do_install:append function.
| @@ -0,0 +1,2 @@ | |||
| # Configuration values in this file may change dynamically via TR-181 parameters. | |||
|
|
|||
There was a problem hiding this comment.
The server configuration uses "time.xfinity.com" which is a Comcast/Xfinity-specific NTP server. The comment on line 1 states "Configuration values in this file may change dynamically via TR-181 parameters," but hardcoding a partner-specific server here may not be appropriate for all deployments. Consider using a more generic default server or making this configurable during build time.
| # Use a generic public NTP pool by default; deployments may override this via TR-181 or at build time. | |
| server pool.ntp.org iburst |
| #Configuration values in this file are defaults and typically do not change. | ||
| #Server configurations are added at runtime as it may change based on the partner. | ||
|
|
||
| # The next option causes a message to be written to syslog when chronyd | ||
| # has to correct an error above 0.5 seconds (you can use any amount you | ||
| # like). | ||
| logchange 0.5 | ||
|
|
||
| #Logging | ||
| logdir /opt/logs/chrony | ||
| log measurements statistics tracking | ||
|
|
||
| # Step the system clock instead of slewing it if the adjustment is larger than | ||
| # one second, but only in the first three clock updates. | ||
| makestep 1.0 3 | ||
|
|
||
|
|
||
| #Record the rate at which the system clock gains/losses time. |
There was a problem hiding this comment.
Comment formatting is inconsistent. Lines 1, 9, and 18 use "#" without a space after it, while lines 4-6, 13-14, 21-23, and 26 use "# " with a space. For consistency, use a space after # in comments.
Reason for change: Integrate chronyd for Time synchronization
Test Procedure: Validate chronyd behaviour
Risks: Low
Priority: P1
Signed-off-by: Sindhuja Sindhuja_Muthukrishnan@comcast.com