Skip to content

feat/DHCP NTP Servers - Enhances time sync with DHCP NTP and custom servers #625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 11, 2025
Merged

feat/DHCP NTP Servers - Enhances time sync with DHCP NTP and custom servers #625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
466bf40
Ensure the mDNS mode is set every time network state changes
IDisposable Jun 18, 2025
087487f
Add custom NTP and HTTP time sync servers
IDisposable Jun 18, 2025
926fcf2
Add support for using DHCP-provided NTP server
IDisposable Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion internal/confparser/confparser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ type testNetworkConfig struct {
LLDPTxTLVs []string `json:"lldp_tx_tlvs,omitempty" one_of:"chassis,port,system,vlan" default:"chassis,port,system,vlan"`
MDNSMode null.String `json:"mdns_mode,omitempty" one_of:"disabled,auto,ipv4_only,ipv6_only" default:"auto"`
TimeSyncMode null.String `json:"time_sync_mode,omitempty" one_of:"ntp_only,ntp_and_http,http_only,custom" default:"ntp_and_http"`
TimeSyncOrdering []string `json:"time_sync_ordering,omitempty" one_of:"http,ntp,ntp_dhcp,ntp_user_provided,ntp_fallback" default:"ntp,http"`
TimeSyncOrdering []string `json:"time_sync_ordering,omitempty" one_of:"http,ntp,ntp_dhcp,ntp_user_provided,http_user_provided" default:"ntp,http"`
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since anyone who ran a version that included the previous code would have gotten the defaults written to their device's config file, we should behave the same way as before until they change the setting (there's still no UI for that, but that's a different PR).

So, the old behavior was to try NTP on the default servers, then HTTP on the default servers. This means that we should interpret the saved config line for ntp,http as if they requested the default servers.

Thus ntp means the prior default NTP servers and we need ntp_user_provided to specify user-supplied values.
The same logic applies to the HTTP URLs, so http refers to the prior default list of HTTP URLs.

Ideally, we probably would like the default ordering to be ntp_dhcp. then ntp, then http but that ship has sailed for anyone running 0.4.x. We might want to change the defaults here for future users.

Thus we added the http_user_provided option and deleted the redundant ntp_fallback option here to match

TimeSyncDisableFallback null.Bool `json:"time_sync_disable_fallback,omitempty" default:"false"`
TimeSyncParallel null.Int `json:"time_sync_parallel,omitempty" default:"4"`
TimeSyncNTPServers []string `json:"time_sync_ntp_servers,omitempty" validate_type:"ipv4_or_ipv6" required_if:"TimeSyncOrdering=ntp_user_provided"`
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

These are where the user's user-provided NTP Server and HTTP URL values can be store. The required_if is likely not completely correct but changing the confparser seems way out of scope here. It will do the right thing iff the user selected only ntp_user_provided in the TimeSyncOrdering but not if they have it included in the string somewhere. I think @ym might know how to express that (includes not equals), but not there now.

TimeSyncHTTPUrls []string `json:"time_sync_http_urls,omitempty" validate_type:"url" required_if:"TimeSyncOrdering=http_user_provided"`
}

func TestValidateConfig(t *testing.T) {
Expand Down
4 changes: 3 additions & 1 deletion internal/network/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,11 @@ type NetworkConfig struct {
LLDPTxTLVs []string `json:"lldp_tx_tlvs,omitempty" one_of:"chassis,port,system,vlan" default:"chassis,port,system,vlan"`
MDNSMode null.String `json:"mdns_mode,omitempty" one_of:"disabled,auto,ipv4_only,ipv6_only" default:"auto"`
TimeSyncMode null.String `json:"time_sync_mode,omitempty" one_of:"ntp_only,ntp_and_http,http_only,custom" default:"ntp_and_http"`
TimeSyncOrdering []string `json:"time_sync_ordering,omitempty" one_of:"http,ntp,ntp_dhcp,ntp_user_provided,ntp_fallback" default:"ntp,http"`
TimeSyncOrdering []string `json:"time_sync_ordering,omitempty" one_of:"http,ntp,ntp_dhcp,ntp_user_provided,http_user_provided" default:"ntp,http"`
TimeSyncDisableFallback null.Bool `json:"time_sync_disable_fallback,omitempty" default:"false"`
TimeSyncParallel null.Int `json:"time_sync_parallel,omitempty" default:"4"`
TimeSyncNTPServers []string `json:"time_sync_ntp_servers,omitempty" validate_type:"ipv4_or_ipv6" required_if:"TimeSyncOrdering=ntp_user_provided"`
TimeSyncHTTPUrls []string `json:"time_sync_http_urls,omitempty" validate_type:"url" required_if:"TimeSyncOrdering=http_user_provided"`
}

func (c *NetworkConfig) GetMDNSMode() *mdns.MDNSListenOptions {
Expand Down
44 changes: 43 additions & 1 deletion internal/network/netif.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type NetworkInterfaceState struct {
ipv6Addr *net.IP
ipv6Addresses []IPv6Address
ipv6LinkLocal *net.IP
ntpAddresses []*net.IP
macAddr *net.HardwareAddr

l *zerolog.Logger
Expand Down Expand Up @@ -76,6 +77,7 @@ func NewNetworkInterfaceState(opts *NetworkInterfaceOptions) (*NetworkInterfaceS
onInitialCheck: opts.OnInitialCheck,
cbConfigChange: opts.OnConfigChange,
config: opts.NetworkConfig,
ntpAddresses: make([]*net.IP, 0),
}

// create the dhcp client
Expand All @@ -89,7 +91,7 @@ func NewNetworkInterfaceState(opts *NetworkInterfaceOptions) (*NetworkInterfaceS
opts.Logger.Error().Err(err).Msg("failed to update network state")
return
}

_ = s.updateNtpServersFromLease(lease)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Every time we get a new DHCP lease, we need to copy over the NTP servers (if any) to the NetworkInterfaceState for access inside the time-sync logic.

_ = s.setHostnameIfNotSame()

opts.OnDhcpLeaseChange(lease)
Expand Down Expand Up @@ -135,6 +137,27 @@ func (s *NetworkInterfaceState) IPv6String() string {
return s.ipv6Addr.String()
}

func (s *NetworkInterfaceState) NtpAddresses() []*net.IP {
return s.ntpAddresses
}

func (s *NetworkInterfaceState) NtpAddressesString() []string {
ntpServers := []string{}

if s != nil {
s.l.Debug().Any("s", s).Msg("getting NTP address strings")

if len(s.ntpAddresses) > 0 {
for _, server := range s.ntpAddresses {
s.l.Debug().IPAddr("server", *server).Msg("converting NTP address")
ntpServers = append(ntpServers, server.String())
}
}
}

return ntpServers
}

func (s *NetworkInterfaceState) MAC() *net.HardwareAddr {
return s.macAddr
}
Expand Down Expand Up @@ -318,6 +341,25 @@ func (s *NetworkInterfaceState) update() (DhcpTargetState, error) {
return dhcpTargetState, nil
}

func (s *NetworkInterfaceState) updateNtpServersFromLease(lease *udhcpc.Lease) error {
if lease != nil && len(lease.NTPServers) > 0 {
s.l.Info().Msg("lease found, updating DHCP NTP addresses")
s.ntpAddresses = make([]*net.IP, 0, len(lease.NTPServers))

for _, ntpServer := range lease.NTPServers {
if ntpServer != nil {
s.l.Info().IPAddr("ntp_server", ntpServer).Msg("NTP server found in lease")
s.ntpAddresses = append(s.ntpAddresses, &ntpServer)
}
}
} else {
s.l.Info().Msg("no NTP servers found in lease")
s.ntpAddresses = make([]*net.IP, 0, len(s.config.TimeSyncNTPServers))
}

return nil
}

func (s *NetworkInterfaceState) CheckAndUpdateDhcp() error {
dhcpTargetState, err := s.update()
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions internal/timesync/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ var defaultHTTPUrls = []string{
// "http://www.msftconnecttest.com/connecttest.txt",
}

func (t *TimeSync) queryAllHttpTime() (now *time.Time) {
chunkSize := 4
httpUrls := t.httpUrls
func (t *TimeSync) queryAllHttpTime(httpUrls []string) (now *time.Time) {
chunkSize := int(t.networkConfig.TimeSyncParallel.ValueOr(4))
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

Honors the setting of the number of parallel time-sync requests (defaults to 4)

t.l.Info().Strs("httpUrls", httpUrls).Int("chunkSize", chunkSize).Msg("querying HTTP URLs")

// shuffle the http urls to avoid always querying the same servers
rand.Shuffle(len(httpUrls), func(i, j int) { httpUrls[i], httpUrls[j] = httpUrls[j], httpUrls[i] })
Expand Down
1 change: 1 addition & 0 deletions internal/timesync/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ var (
},
[]string{"url"},
)

metricNtpServerInfo = promauto.NewGaugeVec(
prometheus.GaugeOpts{
Name: "jetkvm_timesync_ntp_server_info",
Expand Down
30 changes: 24 additions & 6 deletions internal/timesync/ntp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package timesync

import (
"context"
"math/rand/v2"
"strconv"
"time"
Expand All @@ -21,9 +22,9 @@ var defaultNTPServers = []string{
"3.pool.ntp.org",
}

func (t *TimeSync) queryNetworkTime() (now *time.Time, offset *time.Duration) {
chunkSize := 4
ntpServers := t.ntpServers
func (t *TimeSync) queryNetworkTime(ntpServers []string) (now *time.Time, offset *time.Duration) {
chunkSize := int(t.networkConfig.TimeSyncParallel.ValueOr(4))
t.l.Info().Strs("servers", ntpServers).Int("chunkSize", chunkSize).Msg("querying NTP servers")

// shuffle the ntp servers to avoid always querying the same servers
rand.Shuffle(len(ntpServers), func(i, j int) { ntpServers[i], ntpServers[j] = ntpServers[j], ntpServers[i] })
Expand All @@ -46,6 +47,10 @@ type ntpResult struct {

func (t *TimeSync) queryMultipleNTP(servers []string, timeout time.Duration) (now *time.Time, offset *time.Duration) {
results := make(chan *ntpResult, len(servers))

_, cancel := context.WithTimeout(context.Background(), timeout)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Add logic to cancel all outstanding NTP queries when we process a valid reply or end this chunk. This parallels logic in the HTTP path, and might not be desired.

defer cancel()

for _, server := range servers {
go func(server string) {
scopedLogger := t.l.With().
Expand All @@ -66,15 +71,25 @@ func (t *TimeSync) queryMultipleNTP(servers []string, timeout time.Duration) (no
return
}

if response.IsKissOfDeath() {
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

We really need to make sure the NTP server we were talking to didn't tell us to go away. https://support.ntp.org/Dev/KoDResponses

scopedLogger.Warn().
Str("kiss_code", response.KissCode).
Msg("ignoring NTP server kiss of death")
results <- nil
return
}

rtt := float64(response.RTT.Milliseconds())

// set the last RTT
metricNtpServerLastRTT.WithLabelValues(
server,
).Set(float64(response.RTT.Milliseconds()))
).Set(rtt)

// set the RTT histogram
metricNtpServerRttHistogram.WithLabelValues(
server,
).Observe(float64(response.RTT.Milliseconds()))
).Observe(rtt)

// set the server info
metricNtpServerInfo.WithLabelValues(
Expand All @@ -91,10 +106,13 @@ func (t *TimeSync) queryMultipleNTP(servers []string, timeout time.Duration) (no
scopedLogger.Info().
Str("time", now.Format(time.RFC3339)).
Str("reference", response.ReferenceString()).
Str("rtt", response.RTT.String()).
Float64("rtt", rtt).
Str("clockOffset", response.ClockOffset.String()).
Uint8("stratum", response.Stratum).
Msg("NTP server returned time")

cancel()

results <- &ntpResult{
now: now,
offset: &response.ClockOffset,
Expand Down
111 changes: 81 additions & 30 deletions internal/timesync/timesync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@ type TimeSync struct {
syncLock *sync.Mutex
l *zerolog.Logger

ntpServers []string
httpUrls []string
networkConfig *network.NetworkConfig
networkConfig *network.NetworkConfig
dhcpNtpAddresses []string

rtcDevicePath string
rtcDevice *os.File //nolint:unused
Expand Down Expand Up @@ -64,14 +63,13 @@ func NewTimeSync(opts *TimeSyncOptions) *TimeSync {
}

t := &TimeSync{
syncLock: &sync.Mutex{},
l: opts.Logger,
rtcDevicePath: rtcDevice,
rtcLock: &sync.Mutex{},
preCheckFunc: opts.PreCheckFunc,
ntpServers: defaultNTPServers,
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need these two ntpServers and httpUrls in the TimeSync struct as they're available to the time-sync code.

httpUrls: defaultHTTPUrls,
networkConfig: opts.NetworkConfig,
syncLock: &sync.Mutex{},
l: opts.Logger,
dhcpNtpAddresses: []string{},
rtcDevicePath: rtcDevice,
rtcLock: &sync.Mutex{},
preCheckFunc: opts.PreCheckFunc,
networkConfig: opts.NetworkConfig,
}

if t.rtcDevicePath != "" {
Expand All @@ -82,34 +80,42 @@ func NewTimeSync(opts *TimeSyncOptions) *TimeSync {
return t
}

func (t *TimeSync) SetDhcpNtpAddresses(addresses []string) {
t.dhcpNtpAddresses = addresses
}

func (t *TimeSync) getSyncMode() SyncMode {
syncMode := SyncMode{
Ntp: true,
Http: true,
Ordering: []string{"ntp_dhcp", "ntp", "http"},
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This default ordering is a lie, unless there was no stored config, because that will overwrite this defaulting... but just in case, use the DHCP provided NTP servers, then the NTP default servers, then the default HTTP URLs

NtpUseFallback: true,
HttpUseFallback: true,
}
var syncModeString string

if t.networkConfig != nil {
syncModeString = t.networkConfig.TimeSyncMode.String
switch t.networkConfig.TimeSyncMode.String {
case "ntp_only":
syncMode.Http = false
case "http_only":
syncMode.Ntp = false
}

if t.networkConfig.TimeSyncDisableFallback.Bool {
syncMode.NtpUseFallback = false
syncMode.HttpUseFallback = false
}
}

switch syncModeString {
case "ntp_only":
syncMode.Ntp = true
case "http_only":
syncMode.Http = true
default:
syncMode.Ntp = true
syncMode.Http = true
var syncOrdering = t.networkConfig.TimeSyncOrdering
if len(syncOrdering) > 0 {
syncMode.Ordering = syncOrdering
}
}

t.l.Debug().Strs("Ordering", syncMode.Ordering).Bool("Ntp", syncMode.Ntp).Bool("Http", syncMode.Http).Bool("NtpUseFallback", syncMode.NtpUseFallback).Bool("HttpUseFallback", syncMode.HttpUseFallback).Msg("sync mode")

return syncMode
}

func (t *TimeSync) doTimeSync() {
metricTimeSyncStatus.Set(0)
for {
Expand Down Expand Up @@ -154,16 +160,61 @@ func (t *TimeSync) Sync() error {
offset *time.Duration
)

syncMode := t.getSyncMode()

metricTimeSyncCount.Inc()

if syncMode.Ntp {
now, offset = t.queryNetworkTime()
}
syncMode := t.getSyncMode()

if syncMode.Http && now == nil {
now = t.queryAllHttpTime()
Orders:
for _, mode := range syncMode.Ordering {
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

Do the options in the order they specified.

switch mode {
case "ntp_user_provided":
if syncMode.Ntp {
t.l.Info().Msg("using NTP custom servers")
now, offset = t.queryNetworkTime(t.networkConfig.TimeSyncNTPServers)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

NTP servers from the config (will be empty until the UI is coded)

if now != nil {
t.l.Info().Str("source", "NTP").Time("now", *now).Msg("time obtained")
break Orders
}
}
case "ntp_dhcp":
if syncMode.Ntp {
t.l.Info().Msg("using NTP servers from DHCP")
now, offset = t.queryNetworkTime(t.dhcpNtpAddresses)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

NTP servers from the DHCP lease

if now != nil {
t.l.Info().Str("source", "NTP DHCP").Time("now", *now).Msg("time obtained")
break Orders
}
}
case "ntp":
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

Note that ntp means the prior default NTP server list.

if syncMode.Ntp && syncMode.NtpUseFallback {
t.l.Info().Msg("using NTP fallback")
now, offset = t.queryNetworkTime(defaultNTPServers)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

NTP servers from the code-defined defaults (fallbacks)

if now != nil {
t.l.Info().Str("source", "NTP fallback").Time("now", *now).Msg("time obtained")
break Orders
}
}
case "http_user_provided":
if syncMode.Http {
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

Note, that http means the prior HTTP URL default list.

t.l.Info().Msg("using HTTP custom URLs")
now = t.queryAllHttpTime(t.networkConfig.TimeSyncHTTPUrls)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

HTTP URLs from the config (will be empty until the UI is coded)

if now != nil {
t.l.Info().Str("source", "HTTP").Time("now", *now).Msg("time obtained")
break Orders
}
}
case "http":
if syncMode.Http && syncMode.HttpUseFallback {
t.l.Info().Msg("using HTTP fallback")
now = t.queryAllHttpTime(defaultHTTPUrls)
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

HTTP URLs from the code-defined defaults (fallbacks)

if now != nil {
t.l.Info().Str("source", "HTTP fallback").Time("now", *now).Msg("time obtained")
break Orders
}
}
default:
t.l.Warn().Str("mode", mode).Msg("unknown time sync mode, skipping")
}
}

if now == nil {
Expand Down
17 changes: 9 additions & 8 deletions network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,17 @@
// do not block the main thread
go waitCtrlAndRequestDisplayUpdate(true)

if timeSync != nil {
Copy link
Contributor Author

@IDisposable IDisposable Jun 19, 2025

Choose a reason for hiding this comment

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

These nil checks are necessary because we can hit networkStateChanged before we've even finished setting up.

if networkState != nil {
timeSync.SetDhcpNtpAddresses(networkState.NtpAddressesString())
}

timeSync.Sync()

Check failure on line 27 in network.go

View workflow job for this annotation

GitHub Actions / lint 

Error return value of `timeSync.Sync` is not checked (errcheck)
}

// always restart mDNS when the network state changes
if mDNS != nil {
_ = mDNS.SetListenOptions(config.NetworkConfig.GetMDNSMode())
_ = mDNS.SetLocalNames([]string{
networkState.GetHostname(),
networkState.GetFQDN(),
Expand Down Expand Up @@ -54,14 +63,6 @@
OnConfigChange: func(networkConfig *network.NetworkConfig) {
config.NetworkConfig = networkConfig
networkStateChanged()

if mDNS != nil {
_ = mDNS.SetListenOptions(networkConfig.GetMDNSMode())
_ = mDNS.SetLocalNames([]string{
networkState.GetHostname(),
networkState.GetFQDN(),
}, true)
}
},
})

Expand Down
Loading