Skip to content
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.

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

Add D-Bus API for configuring NTP servers of systemd-timesyncd #207

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package main

import (
os_time "github.com/home-assistant/os-agent/time"
"time"

"github.com/coreos/go-systemd/v22/daemon"
Expand Down Expand Up @@ -70,6 +71,7 @@ func main() {
apparmor.InitializeDBus(conn)
cgroup.InitializeDBus(conn)
boards.InitializeDBus(conn, board)
os_time.InitializeDBus(conn)

_, err = daemon.SdNotify(false, daemon.SdNotifyReady)
if err != nil {
Expand Down
175 changes: 175 additions & 0 deletions time/timesyncd.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
package time
Copy link
Member

Choose a reason for hiding this comment

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

To avoid clash with the go time package, I'd go for something else, like timesync.

Suggested change
package time
package timesync

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to have the same prefix for the DBus interfaces controlling other time-related features in the future, and so far we've got it synchronized with package naming. In the end the only clash is in the main file, where it can be simply aliased.

Copy link
Member

Choose a reason for hiding this comment

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

To me this is mostly about filling the gap of missing D-Bus interfaces in systemd. Ideally, we would extend systemd, but this is much more time consuming, so I am fine doing our thing now here.

But, with that in mind, I think it make sense to align with the systemd D-Bus naming: Systemd has two time/date related D-Bus services: org.freedesktop.timesync1 and org.freedesktop.timedate1. What we currently miss is setting the system NTP server, which most likely should be part of the org.freedesktop.timesync1 service.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than the Systemd D-Bus naming, consider naming of our D-Bus interfaces - now it's io.hass.os.Time.Timesyncd, which is implemented in time/timesyncd.go. If we added e.g. interface for timezone setting (leave aside the fact there's some method in org.freedesktop.timedate1 that might be used instead), a logical path would be io.hass.os.Time.Timezone and the implementation would go to time/timezone.go. If the package should be renamed, then we should also adjust D-Bus paths and interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

With the other discussion in mind, it kinda seems what we need to build here is mostly a systemd config file manager. So aligning the naming to the config file names probably make sense 🤔


import (
"fmt"
"github.com/godbus/dbus/v5"
"github.com/godbus/dbus/v5/introspect"
"github.com/godbus/dbus/v5/prop"
"github.com/home-assistant/os-agent/utils/lineinfile"
"regexp"
"strings"

logging "github.com/home-assistant/os-agent/utils/log"
)

const (
objectPath = "/io/hass/os/Time/Timesyncd"
ifaceName = "io.hass.os.Time.Timesyncd"
timesyncdConf = "/etc/systemd/timesyncd.conf"
)

var (
optNTPServer []string
optFallbackNTPServer []string
configFile = lineinfile.LineInFile{FilePath: timesyncdConf}
)
Comment on lines +21 to +25
Copy link

Choose a reason for hiding this comment

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

Global Variables and Configuration File Initialization

The use of global variables (optNTPServer, optFallbackNTPServer) is generally discouraged as it can lead to issues with state management, especially in a multi-threaded environment. Consider encapsulating these within the timesyncd struct to better manage their scope and lifecycle.

Suggest encapsulating global variables within the timesyncd struct:

-type timesyncd struct {
-	conn  *dbus.Conn
-	props *prop.Properties
+type timesyncd struct {
+	conn                *dbus.Conn
+	props               *prop.Properties
+	optNTPServer        []string
+	optFallbackNTPServer []string
}

Committable suggestion was skipped due to low confidence.


type timesyncd struct {
conn *dbus.Conn
props *prop.Properties
}

func getNTPServers() []string {
return getTimesyncdConfigProperty("NTP")
}

func getFallbackNTPServers() []string {
return getTimesyncdConfigProperty("FallbackNTP")
}

func setNTPServer(c *prop.Change) *dbus.Error {
servers, ok := c.Value.([]string)
if !ok {
return dbus.MakeFailedError(fmt.Errorf("invalid type for NTPServer"))
}

value := strings.Join(servers, " ")

if err := setTimesyncdConfigProperty("NTP", value); err != nil {
return dbus.MakeFailedError(err)
}

optNTPServer = servers
return nil
}
Comment on lines +40 to +54
Copy link

Choose a reason for hiding this comment

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

Function setNTPServer Review

This function handles the setting of NTP servers via D-Bus properties. The error handling and type assertion are correctly implemented. However, the function directly modifies a global variable, which could lead to race conditions or other concurrency issues. Consider using a mutex or similar synchronization mechanism if the API is expected to handle concurrent modifications.

Suggest adding synchronization to handle concurrent access:

+import "sync"

+var mutex sync.Mutex

func setNTPServer(c *prop.Change) *dbus.Error {
+	mutex.Lock()
+	defer mutex.Unlock()
	...
}

Committable suggestion was skipped due to low confidence.


func setFallbackNTPServer(c *prop.Change) *dbus.Error {
servers, ok := c.Value.([]string)
if !ok {
return dbus.MakeFailedError(fmt.Errorf("invalid type for FallbackNTPServer"))
}

value := strings.Join(servers, " ")

if err := setTimesyncdConfigProperty("FallbackNTP", value); err != nil {
return dbus.MakeFailedError(err)
}

optFallbackNTPServer = servers
return nil
}

func getTimesyncdConfigProperty(property string) []string {
value, err := configFile.Find(`^\s*(`+property+`=).*$`, `\[Time\]`, true)

var servers []string

if err != nil || value == nil {
return servers
}

matches := regexp.MustCompile(property + `=([^\s#]+(?:\s+[^\s#]+)*)`).FindStringSubmatch(*value)
if len(matches) > 1 {
servers = strings.Split(matches[1], " ")
}

return servers
}

func setTimesyncdConfigProperty(property string, value string) error {
var params = lineinfile.NewPresentParams("NTP=" + value)
params.Regexp, _ = regexp.Compile(`^\s*#?\s*(` + property + `=).*$`)
// Keep it simple, timesyncd.conf only has the [Time] section
params.After = `\[Time\]`
if err := configFile.Present(params); err != nil {
return fmt.Errorf("failed to set %s: %w", property, err)
}

if err := restartTimesyncd(); err != nil {
return fmt.Errorf("failed to restart timesyncd: %w", err)
}

return nil
}

func restartTimesyncd() error {
conn, err := dbus.SystemBus()
if err != nil {
return err
}

obj := conn.Object("org.freedesktop.systemd1", "/org/freedesktop/systemd1")
call := obj.Call("org.freedesktop.systemd1.Manager.RestartUnit", 0, "systemd-timesyncd.service", "replace")
if call.Err != nil {
return call.Err
}

return nil
}

func InitializeDBus(conn *dbus.Conn) {
d := timesyncd{
conn: conn,
}

optNTPServer = getNTPServers()
Copy link
Member

Choose a reason for hiding this comment

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

I guess this could be a bit racy, depending on when OS Agent/NetworkManager hook gets called 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The current implementation only touches the persistent configuration file, so NetworkManager hook doesn't affect it in any way. If we want to read the dynamically obtained NTPs written by the NM hook, maybe using a different method/property would be better.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we even need these getters? There is FallbackNTPServers and SystemNTPServers available on the /org/freedesktop/timesync1 object of the org.freedesktop.timesync1 service.

In a way, this interface to me is mainly a stopgap until systemd-timesyncd has a similar functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those can be amalgamation of multiple configuration files as described in man timesyncd.conf. This interface exposes what is statically configured in the file we'd like to be the source of truth for the configuration. I can imagine the "native" Systemd attributes could be used for sanity checking, raising an issue if the configured value doesn't match what's in the config (which e.g. means user has created extra configuration files). But it would be confusing if you had only the setters, and under some circumstances the getters would return different value than what you have set.

I also doubt that Systemd will ever get similar functionality. There are the runtime NTP servers but those we can't use because we need to apply the configuration before our "configuration daemon" (Supervisor) is started. AFAIK Systemd doesn't have a way to edit configuration files through D-Bus API anywhere, and some things can be only configured only through config files.

Copy link
Member

Choose a reason for hiding this comment

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

Those can be amalgamation of multiple configuration files as described in man timesyncd.conf. This interface exposes what is statically configured in the file we'd like to be the source of truth for the configuration. I can imagine the "native" Systemd attributes could be used for sanity checking, raising an issue if the configured value doesn't match what's in the config (which e.g. means user has created extra configuration files). But it would be confusing if you had only the setters, and under some circumstances the getters would return different value than what you have set.

Right, I definitely would not forward the getter here to D-Bus, that would indeed be confusing.

I am just wonder, if we cannot get away with a more minimal D-Bus interface. Sure, we already have it implemented (with this PR), but there is always maintenance cost.

I also doubt that Systemd will ever get similar functionality. There are the runtime NTP servers but those we can't use because we need to apply the configuration before our "configuration daemon" (Supervisor) is started. AFAIK Systemd doesn't have a way to edit configuration files through D-Bus API anywhere, and some things can be only configured only through config files.

Yeah you probably be right on that there won't be a timesync1 setter. But there is already a systemd-networkd API to set NTP servers:
https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.network1.html#The%20Manager%20Object

I think systemd sees NTP server configuration a network manager thing, just like DNS. And it should be handled and stored by the network manager, and transferred at the appropriate time (the appropriate link is actually ready). I mean, you can imagine a system where you have two network links, each have their own local NTP servers.. You can't simply mix and match in that scenario. Ofc, this won't be the typical HAOS scenario, but it helps to understand how things develop. On the other hand, the system/fallback NTP's will always be global.

Researching a bit more, I came accross this Cockpit issue: cockpit-project/cockpit#7987 (comment)

And the linked systemd issue: systemd/systemd#7593 (comment) and related systemd/systemd#27469. The gist is that systemd-networkd (and I guess with that systemd-timesyncd) lacks a configuration management API. It is all meant to go through files, and that is what it is currently.

Maybe a generic systemd configuration file management API would be better? 🤔 On the other hand, maybe it is better to have OS Agent as a bit of a firewall for this 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I think systemd sees NTP server configuration a network manager thing, just like DNS. And it should be handled and stored by the network manager, and transferred at the appropriate time (the appropriate link is actually ready). I mean, you can imagine a system where you have two network links, each have their own local NTP servers.

Yes, but I think this is not achievable without using systemd-networkd at the same time. Currently we store the network config in NetworkManager's config files, where we lack any mechanism to save per-link user-configured NTP servers, so we'd have to introduce one (i.e. have our own config files/attributes of individual NM connections that'd somehow propagate to timesyncd config). But the complexity of such implementation would heavily outweigh any maintenance cost of the current approach to this problem.

Anyway, if we shall consider multihomed setups which even require different NTP servers on individual links, then I think this discussion turns academic. AFAIK most of the multihomed setups are not what HA considers anywhere, leave aside the fact that even just the NTP config alone seems too advanced for some 😅 We really should think about the real use case, and the way we want to handle NTP configuration on user's side.

I agree though that we can take a different approach on this problem and instead of exposing "time subsystem" APIs, we could have "system configuration" instead, if that makes more sense to you.

optFallbackNTPServer = getFallbackNTPServers()

propsSpec := map[string]map[string]*prop.Prop{
ifaceName: {
"NTPServer": {
Value: optNTPServer,
Writable: true,
Emit: prop.EmitTrue,
Callback: setNTPServer,
},
"FallbackNTPServer": {
Value: optFallbackNTPServer,
Writable: true,
Emit: prop.EmitTrue,
Callback: setFallbackNTPServer,
},
},
}

props, err := prop.Export(conn, objectPath, propsSpec)
if err != nil {
logging.Critical.Panic(err)
}
d.props = props

err = conn.Export(d, objectPath, ifaceName)
if err != nil {
logging.Critical.Panic(err)
}

node := &introspect.Node{
Name: objectPath,
Interfaces: []introspect.Interface{
introspect.IntrospectData,
prop.IntrospectData,
{
Name: ifaceName,
Methods: introspect.Methods(d),
Properties: props.Introspection(ifaceName),
},
},
}

err = conn.Export(introspect.NewIntrospectable(node), objectPath, "org.freedesktop.DBus.Introspectable")
if err != nil {
logging.Critical.Panic(err)
}

logging.Info.Printf("Exposing object %s with interface %s ...", objectPath, ifaceName)
}
sairon marked this conversation as resolved.
Show resolved Hide resolved
Loading