diff --git a/go.mod b/go.mod index 050bd22b1b..61f49b33d7 100644 --- a/go.mod +++ b/go.mod @@ -30,6 +30,7 @@ require ( github.com/distribution/reference v0.6.0 github.com/fsnotify/fsnotify v1.9.0 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 + github.com/godbus/dbus/v5 v5.2.2 github.com/golangci/golangci-lint v1.62.0 github.com/google/go-cmp v0.7.0 github.com/google/goexpect v0.0.0-20210430020637-ab937bf7fd6f @@ -143,7 +144,6 @@ require ( github.com/go-openapi/validate v0.25.2 // indirect github.com/go-task/slim-sprig/v3 v3.0.0 // indirect github.com/go-viper/mapstructure/v2 v2.5.0 // indirect - github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/golangci/go-printf-func-name v0.1.0 // indirect github.com/golangci/modinfo v0.3.4 // indirect github.com/golangci/plugin-module-register v0.1.1 // indirect diff --git a/pkg/daemon/certificate_writer.go b/pkg/daemon/certificate_writer.go index 447d9fcb0a..76a73ad687 100644 --- a/pkg/daemon/certificate_writer.go +++ b/pkg/daemon/certificate_writer.go @@ -295,8 +295,14 @@ func (dn *Daemon) syncControllerConfigHandler(key string) error { if controllerConfig.Annotations[ctrlcommon.ServiceCARotateAnnotation] == ctrlcommon.ServiceCARotateTrue && oldAnno != controllerConfig.Annotations[ctrlcommon.ServiceCARotateAnnotation] && cmErr == nil && kubeConfigDiff && !allCertsThere && !dn.deferKubeletRestart { if len(onDiskKC.Clusters[0].Cluster.CertificateAuthorityData) > 0 { logSystem("restarting kubelet due to server-ca rotation") - if err := runCmdSync("systemctl", "stop", "kubelet"); err != nil { - return err + systemdConnection, err := dn.systemdManager.NewConnection(context.Background()) + if err != nil { + return fmt.Errorf("error creating connection to systemd: %w", err) + } + defer systemdConnection.Close() + + if err := systemdConnection.Stop(context.Background(), "kubelet"); err != nil { + return fmt.Errorf("could not stop kubelet for server-ca rotation. Error: %v", err) } f, err := os.ReadFile("/var/lib/kubelet/kubeconfig") if err != nil && os.IsNotExist(err) { @@ -323,12 +329,12 @@ func (dn *Daemon) syncControllerConfigHandler(key string) error { return err } - if err := runCmdSync("systemctl", "daemon-reload"); err != nil { - return err + if err := systemdConnection.ReloadDaemon(context.Background()); err != nil { + return fmt.Errorf("could not reload systemd after kubelet stop for server-ca rotation. Error: %v", err) } - if err := runCmdSync("systemctl", "start", "kubelet"); err != nil { - return err + if err := systemdConnection.Start(context.Background(), "kubelet"); err != nil { + return fmt.Errorf("could not start kubelet after stopping it for server-ca rotation. Error: %v", err) } } diff --git a/pkg/daemon/config_drift_monitor_test.go b/pkg/daemon/config_drift_monitor_test.go index ad5908a0e6..591357b494 100644 --- a/pkg/daemon/config_drift_monitor_test.go +++ b/pkg/daemon/config_drift_monitor_test.go @@ -496,9 +496,18 @@ func (tc *configDriftMonitorTestCase) writeIgnitionConfig(t *testing.T, ignConfi return fmt.Errorf("could not write ignition config files: %w", err) } - // Write systemd units the same way the MCD does. - if err := writeUnits(ignConfig.Systemd.Units, tc.systemdPath, true); err != nil { - return fmt.Errorf("could not write systemd units: %w", err) + // Use a mock systemd connection for testing + // Create mock units for all units in the config + mockUnits := make(map[string]*mockUnitState) + for _, unit := range ignConfig.Systemd.Units { + mockUnits[unit.Name] = newMockUnitState(unit.Name) + } + mockConn := newMockSystemdConnection(mockUnits) + + for _, unit := range ignConfig.Systemd.Units { + if err := writeUnit(mockConn, unit, tc.systemdPath, true); err != nil { + return fmt.Errorf("could not write unit: %w", err) + } } return nil diff --git a/pkg/daemon/constants/constants.go b/pkg/daemon/constants/constants.go index 03411fb0ec..c20717210d 100644 --- a/pkg/daemon/constants/constants.go +++ b/pkg/daemon/constants/constants.go @@ -116,7 +116,7 @@ const ( RHCOSDefaultSSHKeyPath = CoreUserSSHPath + "/authorized_keys.d/ignition" // CRIOServiceName is used to specify reloads and restarts of the CRI-O service - CRIOServiceName = "crio" + CRIOServiceName = "crio.service" // DaemonReloadCommand is used to specify reloads and restarts of the systemd manager configuration DaemonReloadCommand = "daemon-reload" diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 3957146681..955107eb72 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -165,6 +165,9 @@ type Daemon struct { // Abstraction for running commands against the OS cmdRunner CommandRunner + // Abstraction for interacting with systemd + systemdManager SystemdManager + // Bare minimal podman client podmanInterface PodmanInterface } @@ -315,9 +318,10 @@ func New( var nodeUpdaterClient *RpmOstreeClient cmdRunner := &CommandRunnerOS{} podmanInterface := NewPodmanExec(cmdRunner) + systemdManager := NewSystemdManagerDefault() // Only pull the osImageURL from OSTree when we are on RHCOS or FCOS if hostos.IsCoreOSVariant() { - nodeUpdaterClientVal := NewNodeUpdaterClient(cmdRunner, podmanInterface) + nodeUpdaterClientVal := NewNodeUpdaterClient(cmdRunner, podmanInterface, systemdManager) nodeUpdaterClient = &nodeUpdaterClientVal err := nodeUpdaterClient.Initialize() if err != nil { @@ -356,6 +360,7 @@ func New( irreconcilableReporter: NewNoOpIrreconcilableReporterImpl(), cmdRunner: cmdRunner, podmanInterface: podmanInterface, + systemdManager: systemdManager, }, nil } @@ -1170,7 +1175,7 @@ func (dn *Daemon) syncNodeHypershift(key string) error { if ctrlcommon.InSlice(postConfigChangeActionReloadCrio, actions) { serviceName := constants.CRIOServiceName - if err := reloadService(serviceName); err != nil { + if err := dn.systemdManager.DoConnection(context.Background(), SystemdReload(serviceName)); err != nil { return fmt.Errorf("could not apply update: reloading %s configuration failed. Error: %w", serviceName, err) } klog.Infof("%s config reloaded successfully! Desired config %s has been applied, skipping reboot", serviceName, desiredConfig.Name) diff --git a/pkg/daemon/file_writers.go b/pkg/daemon/file_writers.go index 8df8f44089..db01401fc1 100644 --- a/pkg/daemon/file_writers.go +++ b/pkg/daemon/file_writers.go @@ -2,6 +2,7 @@ package daemon import ( "bufio" + "context" "fmt" "os" "os/exec" @@ -9,6 +10,7 @@ import ( "path/filepath" "strconv" "strings" + "time" ign3types "github.com/coreos/ignition/v2/config/v3_5/types" "github.com/google/renameio" @@ -265,7 +267,7 @@ func writeFiles(files []ign3types.File, skipCertificateWrite bool) error { } // writeUnit writes a systemd unit and its dropins to disk -func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error { +func writeUnit(systemdConnection SystemdConnection, u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error { if err := writeDropins(u, systemdRoot, isCoreOSVariant); err != nil { return err } @@ -302,13 +304,16 @@ func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error // If the unit is currently enabled, disable it before overwriting since we might be // changing its WantedBy= or RequiredBy= directive (see OCPBUGS-33694). Later code will // re-enable the new unit as directed by the MachineConfig. - cmd := exec.Command("systemctl", "is-enabled", u.Name) - out, _ := cmd.CombinedOutput() - if cmd.ProcessState.ExitCode() == 0 && strings.TrimSpace(string(out)) == "enabled" { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + isEnabled, err := systemdConnection.IsEnabled(ctx, u.Name) + if err != nil { + return fmt.Errorf("failed to check if %q is enabled: %w", u.Name, err) + } + if isEnabled { klog.Infof("Disabling systemd unit %s before re-writing it", u.Name) - disableOut, err := exec.Command("systemctl", "disable", u.Name).CombinedOutput() - if err != nil { - return fmt.Errorf("disabling %s failed: %w (output: %s)", u.Name, err, string(disableOut)) + if err := systemdConnection.Disable(ctx, u.Name); err != nil { + return fmt.Errorf("failed to disable %q: %w", u.Name, err) } } if err := writeFileAtomicallyWithDefaults(fpath, []byte(*u.Contents)); err != nil { @@ -340,17 +345,6 @@ func unitHasContent(u ign3types.Unit) bool { return u.Contents != nil && *u.Contents != "" } -// writeUnits writes systemd units and their dropins to disk -func writeUnits(units []ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error { - for _, u := range units { - if err := writeUnit(u, systemdRoot, isCoreOSVariant); err != nil { - return err - } - } - - return nil -} - func lookupUID(username string) (int, error) { osUser, err := user.Lookup(username) if err != nil { diff --git a/pkg/daemon/pinned_image_set.go b/pkg/daemon/pinned_image_set.go index 460740cf1b..78505699db 100644 --- a/pkg/daemon/pinned_image_set.go +++ b/pkg/daemon/pinned_image_set.go @@ -119,6 +119,9 @@ type PinnedImageSetManager struct { queue workqueue.TypedRateLimitingInterface[string] fgHandler ctrlcommon.FeatureGatesHandler + // Abstraction for interacting with systemd + systemdManager SystemdManager + // mutex protects cancelFn mu sync.Mutex cancelFn context.CancelFunc @@ -150,6 +153,7 @@ func NewPinnedImageSetManager( registryCfgPath: registryCfgPath, prefetchTimeout: prefetchTimeout, minStorageAvailableBytes: minStorageAvailableBytes, + systemdManager: NewSystemdManagerDefault(), fgHandler: fgHandler, queue: workqueue.NewTypedRateLimitingQueueWithConfig[string]( workqueue.DefaultTypedControllerRateLimiter[string](), @@ -286,7 +290,7 @@ func (p *PinnedImageSetManager) syncMachineConfigPools(ctx context.Context, pool // write config and reload crio last to allow a window for kubelet to gc // images in an emergency - if err := ensureCrioPinnedImagesConfigFile(crioPinnedImagesDropInFilePath, imageNames); err != nil { + if err := p.ensureCrioPinnedImagesConfigFile(crioPinnedImagesDropInFilePath, imageNames); err != nil { klog.Errorf("failed to write crio config file: %v", err) return err } @@ -494,7 +498,7 @@ func (p *PinnedImageSetManager) checkImagePayloadStorage(ctx context.Context, im } // ensureCrioPinnedImagesConfigFile ensures the crio config file is up to date with the pinned images. -func ensureCrioPinnedImagesConfigFile(path string, imageNames []string) error { +func (p *PinnedImageSetManager) ensureCrioPinnedImagesConfigFile(path string, imageNames []string) error { cfgExists, err := hasConfigFile(path) if err != nil { return fmt.Errorf("failed to check crio config file: %w", err) @@ -504,7 +508,7 @@ func ensureCrioPinnedImagesConfigFile(path string, imageNames []string) error { if err := deleteCrioConfigFile(); err != nil { return fmt.Errorf("failed to remove CRI-O config file: %w", err) } - return crioReload() + return p.crioReload() } else if len(imageNames) == 0 { return nil } @@ -528,7 +532,7 @@ func ensureCrioPinnedImagesConfigFile(path string, imageNames []string) error { if err := writeFiles([]ign3types.File{ignFile}, true); err != nil { return fmt.Errorf("failed to write CRIO config file: %w", err) } - return crioReload() + return p.crioReload() } klog.Infof("CRI-O config file is up to date, no reload required") @@ -1082,7 +1086,7 @@ func (p *PinnedImageSetManager) deleteMachineConfigPool(obj interface{}) { return } - crioReload() + p.crioReload() } func (p *PinnedImageSetManager) enqueue(pool *mcfgv1.MachineConfigPool) { @@ -1166,6 +1170,15 @@ func (p *PinnedImageSetManager) getImageSize(ctx context.Context, imageName, aut return totalSize, nil } +func (p *PinnedImageSetManager) crioReload() error { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + if err := p.systemdManager.DoConnection(ctx, SystemdReload(constants.CRIOServiceName)); err != nil { + return fmt.Errorf("could not apply update: reloading %s configuration failed. Error: %w", constants.CRIOServiceName, err) + } + return nil +} + // ensurePullImage first checks if the image exists locally and then will attempt to pull // the image from the container runtime with a retry/backoff. func ensurePullImage(ctx context.Context, client *cri.Client, backoff wait.Backoff, image string, authConfig *runtimeapi.AuthConfig) error { @@ -1234,14 +1247,6 @@ func uniqueSortedImageNames(images []mcfgv1.PinnedImageRef) []string { return unique } -func crioReload() error { - serviceName := constants.CRIOServiceName - if err := reloadService(serviceName); err != nil { - return fmt.Errorf("could not apply update: reloading %s configuration failed. Error: %w", serviceName, err) - } - return nil -} - func deleteCrioConfigFile() error { // remove the crio config file if err := os.Remove(crioPinnedImagesDropInFilePath); err != nil { diff --git a/pkg/daemon/pinned_image_set_test.go b/pkg/daemon/pinned_image_set_test.go index ed4790374b..4433fbb0de 100644 --- a/pkg/daemon/pinned_image_set_test.go +++ b/pkg/daemon/pinned_image_set_test.go @@ -571,11 +571,16 @@ func TestEnsureCrioPinnedImagesConfigFile(t *testing.T) { testCfgPath := filepath.Join(tmpDir, "50-pinned-images") err = os.WriteFile(testCfgPath, newCfgBytes, 0644) require.NoError(err) - err = ensureCrioPinnedImagesConfigFile(testCfgPath, imageNames) + + pinnedImageSetManager := PinnedImageSetManager{ + systemdManager: newMockSystemdManager(nil), + } + + err = pinnedImageSetManager.ensureCrioPinnedImagesConfigFile(testCfgPath, imageNames) require.NoError(err) imageNames = append(imageNames, "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd") - err = ensureCrioPinnedImagesConfigFile(testCfgPath, imageNames) + err = pinnedImageSetManager.ensureCrioPinnedImagesConfigFile(testCfgPath, imageNames) require.ErrorIs(err, os.ErrPermission) // this error is from atomic writer attempting to write. } diff --git a/pkg/daemon/rpm-ostree.go b/pkg/daemon/rpm-ostree.go index 87674c043c..81e0bac7ec 100644 --- a/pkg/daemon/rpm-ostree.go +++ b/pkg/daemon/rpm-ostree.go @@ -1,12 +1,14 @@ package daemon import ( + "context" "encoding/json" "fmt" "os" "path/filepath" "reflect" "strings" + "time" "github.com/containers/image/v5/signature" rpmostreeclient "github.com/coreos/rpmostree-client-go/pkg/client" @@ -27,14 +29,16 @@ type RpmOstreeClient struct { client rpmostreeclient.Client commandRunner CommandRunner podmanInterface PodmanInterface + systemdManager SystemdManager } // NewNodeUpdaterClient is a wrapper to create an RpmOstreeClient -func NewNodeUpdaterClient(commandRunner CommandRunner, podmanInterface PodmanInterface) RpmOstreeClient { +func NewNodeUpdaterClient(commandRunner CommandRunner, podmanInterface PodmanInterface, systemdManager SystemdManager) RpmOstreeClient { return RpmOstreeClient{ client: rpmostreeclient.NewClient("machine-config-daemon"), commandRunner: commandRunner, podmanInterface: podmanInterface, + systemdManager: systemdManager, } } @@ -46,7 +50,7 @@ func runRpmOstree(args ...string) error { } // See https://bugzilla.redhat.com/show_bug.cgi?id=2111817 -func bug2111817Workaround() error { +func (r *RpmOstreeClient) bug2111817Workaround() error { targetUnit := "/run/systemd/system/rpm-ostreed.service.d/bug2111817.conf" // Do nothing if the file exists if _, err := os.Stat(targetUnit); err == nil { @@ -62,21 +66,24 @@ InaccessiblePaths= if err := writeFileAtomicallyWithDefaults(targetUnit, []byte(dropin)); err != nil { return err } - if err := runCmdSync("systemctl", "daemon-reload"); err != nil { - return err + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + if err := r.systemdManager.DoConnection(ctx, SystemdReloadDaemon()); err != nil { + return fmt.Errorf("failed to reload daemon for rpm-ostree 2111817 bug : %w", err) } klog.Infof("Enabled workaround for bug 2111817") return nil } func (r *RpmOstreeClient) Initialize() error { - if err := bug2111817Workaround(); err != nil { + if err := r.bug2111817Workaround(); err != nil { return err } // Ensure the temporal ostree dropin doesn't exist // It shouldn't, but it's possible if the MCD container // suddenly died before rpm-ostree rebase finished - if err := cleanupTemporalOstreePolicyFiles(); err != nil { + if err := r.cleanupTemporalOstreePolicyFiles(); err != nil { return err } @@ -110,16 +117,6 @@ func (r *RpmOstreeClient) GetBootedAndStagedDeployment() (*rpmostreeclient.Deplo return booted, staged, nil } -// GetStatus returns multi-line human-readable text describing system status -func (r *RpmOstreeClient) GetStatus() (string, error) { - output, err := r.commandRunner.RunGetOut("rpm-ostree", "status") - if err != nil { - return "", err - } - - return string(output), nil -} - // GetBootedOSImageURL returns the image URL as well as the OSTree version(for logging) and the ostree commit (for comparisons) // Returns the empty string if the host doesn't have a custom origin that matches pivot:// // (This could be the case for e.g. FCOS, or a future RHCOS which comes not-pivoted by default) @@ -212,7 +209,7 @@ func (r *RpmOstreeClient) RebaseLayeredFromContainerStorage(podmanImageInfo *Pod defer func() { // Call the cleanup always, just in case there are left-overs of // a previous killed MCD (unlikely, but possible) - if err := cleanupTemporalOstreePolicyFiles(); err != nil { + if err := r.cleanupTemporalOstreePolicyFiles(); err != nil { klog.Errorf("Error deleting temporary MCD temporal policy %v", err) } }() @@ -298,7 +295,11 @@ func (r *RpmOstreeClient) patchPoliciesForContainerStorage(podmanImageInfo *Podm return err } - if err := writeTemporalOstreePolicyFileDropin(); err != nil { + // Give systemd commands 60 seconds to finish + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + if err := r.writeTemporalOstreePolicyFileDropin(ctx); err != nil { return err } @@ -328,7 +329,7 @@ func (r *RpmOstreeClient) generateTransportPolicyKeyForReference(podmanImageInfo // This allows rpm-ostree to use the patched policy without modifying the system's // policy file directly. After writing the drop-in, the function reloads systemd and // restarts rpm-ostreed to apply the changes. -func writeTemporalOstreePolicyFileDropin() error { +func (r *RpmOstreeClient) writeTemporalOstreePolicyFileDropin(ctx context.Context) error { // Create a temporal dropin to mount the temporal policy into rpm-ostreed process if err := writeFileAtomicallyWithDefaults( rpmOstreeTemporalDropinFile, @@ -339,20 +340,24 @@ func writeTemporalOstreePolicyFileDropin() error { ); err != nil { return err } - return systemdRpmOstreeReload() + return r.systemdRpmOstreeReload(ctx) } // cleanupTemporalOstreePolicyFiles removes the generated temporal files (systemd drop-in // and temporal policy.json) created by writeTemporalOstreePolicyFileDropin, restoring // rpm-ostreed to use the original system policy file. After removing the files, the // function reloads systemd and restarts rpm-ostreed to apply the changes. -func cleanupTemporalOstreePolicyFiles() error { +func (r *RpmOstreeClient) cleanupTemporalOstreePolicyFiles() error { + // Give systemd commands 60 seconds to finish + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + err := os.Remove(rpmOstreeTemporalDropinFile) if err != nil && !os.IsNotExist(err) { return err } else if err == nil { // The file existed: Reload - if err := systemdRpmOstreeReload(); err != nil { + if err := r.systemdRpmOstreeReload(ctx); err != nil { return err } } @@ -366,12 +371,18 @@ func cleanupTemporalOstreePolicyFiles() error { // systemdRpmOstreeReload notifies systemd of unit configuration changes and restarts // the rpm-ostreed service if it's currently running. -func systemdRpmOstreeReload() error { +func (r *RpmOstreeClient) systemdRpmOstreeReload(ctx context.Context) error { + conn, err := r.systemdManager.NewConnection(ctx) + if err != nil { + return fmt.Errorf("error creating connection to systemd: %w", err) + } + defer conn.Close() + // Tell systemd that there are changes in the units - if err := runCmdSync("systemctl", "daemon-reload"); err != nil { + if err := conn.ReloadDaemon(ctx); err != nil { return err } // In case rpm-ostreed is running restart it to take the latest config - return runCmdSync("systemctl", "try-restart", "rpm-ostreed") + return conn.TryRestart(ctx, "rpm-ostreed") } diff --git a/pkg/daemon/systemd.go b/pkg/daemon/systemd.go new file mode 100644 index 0000000000..bd6d4888b9 --- /dev/null +++ b/pkg/daemon/systemd.go @@ -0,0 +1,490 @@ +// Package daemon provides systemd integration for the Machine Config Daemon. +// It implements a factory pattern for managing systemd connections and operations. +// +// The factory pattern allows for efficient connection reuse: +// - Use SystemdManager.NewConnection() for multiple operations on a single connection +// - Use SystemdManager.DoConnection() for one-off operations with automatic cleanup +// +// All systemd unit operations automatically normalize unit names to include proper +// suffixes (e.g., "crio" becomes "crio.service") to comply with D-Bus requirements. +package daemon + +import ( + "context" + "fmt" + systemddbus "github.com/coreos/go-systemd/v22/dbus" + "github.com/godbus/dbus/v5" + "maps" + "strings" + "sync" +) + +// NormalizeSystemdUnitNames ensures unit names have valid systemd suffixes. +// D-Bus requires the full unit name (e.g., "crio.service") while systemctl +// accepts short names (e.g., "crio"). This function adds ".service" if no +// recognized suffix is present. +// +// Valid systemd unit suffixes are: +// +// .service, .socket, .device, .mount, .automount, .swap, +// .target, .path, .timer, .slice, .scope +func NormalizeSystemdUnitNames(units ...string) []string { + validSuffixes := []string{ + ".service", + ".socket", + ".device", + ".mount", + ".automount", + ".swap", + ".target", + ".path", + ".timer", + ".slice", + ".scope", + } + + normalized := make([]string, len(units)) + for i, unit := range units { + hasValidSuffix := false + for _, suffix := range validSuffixes { + if strings.HasSuffix(unit, suffix) { + hasValidSuffix = true + break + } + } + + if hasValidSuffix { + normalized[i] = unit + } else { + // No recognized suffix, assume it's a service unit + normalized[i] = unit + ".service" + } + } + + return normalized +} + +// SystemdConnection represents an active connection to systemd that can perform multiple operations +// without reconnecting. Users should call Close() when done to release resources. +type SystemdConnection interface { + // Enable enables one or more systemd units. + Enable(ctx context.Context, force bool, units ...string) error + + // Disable disables one or more systemd units. + Disable(ctx context.Context, units ...string) error + + // IsEnabled checks if a systemd unit is enabled. + IsEnabled(ctx context.Context, unit string) (bool, error) + + // TryRestart tries to restart a systemd unit (only if it's already running). + TryRestart(ctx context.Context, unit string) error + + // Restart restarts a systemd unit. + Restart(ctx context.Context, unit string) error + + // Start starts a systemd unit. + Start(ctx context.Context, unit string) error + + // Stop stops a systemd unit. + Stop(ctx context.Context, unit string) error + + // Reload reloads a systemd unit. + Reload(ctx context.Context, unit string) error + + // Preset resets a systemd unit to its preset state. + Preset(ctx context.Context, unit string) error + + // ReloadDaemon reloads the systemd daemon configuration. + ReloadDaemon(ctx context.Context) error + + // ListUnits retrieves all currently loaded systemd units and returns them as a map indexed by unit name. + ListUnits(ctx context.Context) (map[string]systemddbus.UnitStatus, error) + + // Close closes the systemd connection and releases resources. + Close() +} + +// SystemdManager is a factory for creating systemd connections. +type SystemdManager interface { + // NewConnection creates a new connection to systemd for multiple operations + // that benefit from connection reuse. Caller must call Close() when done. + NewConnection(context.Context) (SystemdConnection, error) + + // DoConnection performs a one-off operation with automatic connection cleanup. + // The connection is automatically closed after the function executes. + DoConnection(ctx context.Context, fn SystemdManagerDoConnectionFn) error +} + +// SystemdManagerDefault is the default implementation of SystemdManager. +type SystemdManagerDefault struct{} + +// NewSystemdManagerDefault creates a new SystemdManagerDefault +func NewSystemdManagerDefault() *SystemdManagerDefault { + return &SystemdManagerDefault{} +} + +// NewConnection creates a new connection to systemd +func (s *SystemdManagerDefault) NewConnection(_ context.Context) (SystemdConnection, error) { + return &systemdConnectionImpl{}, nil +} + +// SystemdManagerDoConnectionFn is a function type for operations that use a SystemdConnection. +// Used with DoConnection for one-off operations with automatic cleanup. +type SystemdManagerDoConnectionFn func(ctx context.Context, conn SystemdConnection) error + +// DoConnection creates a new connection, executes the provided function, and automatically +// closes the connection. Use this for one-off operations. +func (s *SystemdManagerDefault) DoConnection(ctx context.Context, fn SystemdManagerDoConnectionFn) error { + conn, err := s.NewConnection(ctx) + if err != nil { + return err + } + defer conn.Close() + return fn(ctx, conn) +} + +// systemdConnectionImpl implements SystemdConnection. +type systemdConnectionImpl struct { + conn *systemddbus.Conn + connMutex sync.RWMutex +} + +func (s *systemdConnectionImpl) connect(ctx context.Context) (*systemddbus.Conn, error) { + var err error + s.connMutex.Lock() + defer s.connMutex.Unlock() + if s.conn != nil { + return s.conn, nil + } + logSystem("Creating new systemd D-Bus connection") + s.conn, err = systemddbus.NewSystemdConnectionContext(ctx) + if err != nil { + err = fmt.Errorf("failed to connect to systemd: %w", err) + } + logSystem("Created new systemd D-Bus connection") + + return s.conn, err +} + +// Close closes the systemd connection +func (s *systemdConnectionImpl) Close() { + s.connMutex.Lock() + defer s.connMutex.Unlock() + if s.conn != nil { + logSystem("Closing systemd D-Bus connection") + s.conn.Close() + } +} + +// ReloadDaemon reloads the systemd daemon configuration +func (s *systemdConnectionImpl) ReloadDaemon(ctx context.Context) error { + return s.doWithConnection(ctx, func(conn *systemddbus.Conn) error { + logSystem("Reloading systemd daemon configuration") + if err := conn.ReloadContext(ctx); err != nil { + return fmt.Errorf("failed to reload systemd daemon: %w", err) + } + logSystem("Reloaded systemd daemon configuration") + return nil + }) +} + +// SystemdReloadDaemon returns a function that reloads the systemd daemon configuration. +// Use with DoConnection for one-off systemd daemon reload operations. +func SystemdReloadDaemon() SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.ReloadDaemon(ctx) + } +} + +// SystemdEnable returns a function that enables one or more systemd units. +// Use with DoConnection for one-off enable operations. +func SystemdEnable(force bool, units ...string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Enable(ctx, force, units...) + } +} + +// SystemdDisable returns a function that disables one or more systemd units. +// Use with DoConnection for one-off disable operations. +func SystemdDisable(units ...string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Disable(ctx, units...) + } +} + +// SystemdIsEnabled returns a function that checks if a systemd unit is enabled. +// The result is stored in the provided enabled pointer. +// Use with DoConnection for one-off check operations. +func SystemdIsEnabled(unit string, enabled *bool) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + result, err := conn.IsEnabled(ctx, unit) + if err != nil { + return err + } + *enabled = result + return nil + } +} + +// SystemdTryRestart returns a function that tries to restart a systemd unit. +// The unit is only restarted if it's already running. +// Use with DoConnection for one-off try-restart operations. +func SystemdTryRestart(unit string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.TryRestart(ctx, unit) + } +} + +// SystemdRestart returns a function that restarts a systemd unit. +// Use with DoConnection for one-off restart operations. +func SystemdRestart(unit string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Restart(ctx, unit) + } +} + +// SystemdStart returns a function that starts a systemd unit. +// Use with DoConnection for one-off start operations. +func SystemdStart(unit string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Start(ctx, unit) + } +} + +// SystemdStop returns a function that stops a systemd unit. +// Use with DoConnection for one-off stop operations. +func SystemdStop(unit string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Stop(ctx, unit) + } +} + +// SystemdReload returns a function that reloads a systemd unit. +// Use with DoConnection for one-off reload operations. +func SystemdReload(unit string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Reload(ctx, unit) + } +} + +// SystemdPreset returns a function that resets a systemd unit to its preset state. +// Use with DoConnection for one-off preset operations. +func SystemdPreset(unit string) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + return conn.Preset(ctx, unit) + } +} + +// SystemdListUnits returns a function that retrieves all currently loaded systemd units. +// The units are copied into the provided result map indexed by unit name. +// Use with DoConnection for one-off list units operations. +func SystemdListUnits(result map[string]systemddbus.UnitStatus) SystemdManagerDoConnectionFn { + return func(ctx context.Context, conn SystemdConnection) error { + units, err := conn.ListUnits(ctx) + if err != nil { + return err + } + maps.Copy(result, units) + return nil + } +} + +// Enable enables one or more systemd units. Unit names are automatically normalized. +func (s *systemdConnectionImpl) Enable(ctx context.Context, force bool, units ...string) error { + return s.doWithConnection(ctx, func(conn *systemddbus.Conn) error { + normalizedUnits := NormalizeSystemdUnitNames(units...) + logSystem("Enabling systemd units %v", normalizedUnits) + if _, _, err := conn.EnableUnitFilesContext(ctx, normalizedUnits, false, force); err != nil { + return fmt.Errorf("enabling systemd units %v: %w", normalizedUnits, err) + } + logSystem("Enabled systemd units %v", normalizedUnits) + return nil + }) +} + +// Disable disables one or more systemd units. Unit names are automatically normalized. +func (s *systemdConnectionImpl) Disable(ctx context.Context, units ...string) error { + return s.doWithConnection(ctx, func(conn *systemddbus.Conn) error { + normalizedUnits := NormalizeSystemdUnitNames(units...) + logSystem("Disabling systemd units %v", normalizedUnits) + if _, err := conn.DisableUnitFilesContext(ctx, normalizedUnits, false); err != nil { + return fmt.Errorf("disabling systemd units %v: %w", normalizedUnits, err) + } + logSystem("Disabled systemd units %v", normalizedUnits) + return nil + }) +} + +// TryRestart tries to restart a systemd unit (only if it's already running). Unit name is automatically normalized. +func (s *systemdConnectionImpl) TryRestart(ctx context.Context, unit string) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Try-restarting systemd unit %q", normalizedName) + if err := s.doWithConnection(ctx, doSync(ctx, func(c chan string, conn *systemddbus.Conn) error { + _, err := conn.TryRestartUnitContext(ctx, normalizedName, "replace", c) + return err + })); err != nil { + return fmt.Errorf("try-restarting systemd unit %q: %w", normalizedName, err) + } + logSystem("Try-restarted systemd unit %q", normalizedName) + return nil +} + +// Restart restarts a systemd unit. Unit name is automatically normalized. +func (s *systemdConnectionImpl) Restart(ctx context.Context, unit string) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Restarting systemd unit %q", normalizedName) + if err := s.doWithConnection(ctx, doSync(ctx, func(c chan string, conn *systemddbus.Conn) error { + _, err := conn.RestartUnitContext(ctx, normalizedName, "replace", c) + return err + })); err != nil { + return fmt.Errorf("restarting systemd unit %q: %w", normalizedName, err) + } + logSystem("Restarted systemd unit %q", normalizedName) + return nil +} + +// Start starts a systemd unit. Unit name is automatically normalized. +func (s *systemdConnectionImpl) Start(ctx context.Context, unit string) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Starting systemd unit %q", normalizedName) + if err := s.doWithConnection(ctx, doSync(ctx, func(c chan string, conn *systemddbus.Conn) error { + _, err := conn.StartUnitContext(ctx, normalizedName, "replace", c) + return err + })); err != nil { + return fmt.Errorf("starting systemd unit %q: %w", normalizedName, err) + } + logSystem("Started systemd unit %q", normalizedName) + return nil +} + +// Stop stops a systemd unit. Unit name is automatically normalized. +func (s *systemdConnectionImpl) Stop(ctx context.Context, unit string) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Stopping systemd unit %q", normalizedName) + if err := s.doWithConnection(ctx, doSync(ctx, func(c chan string, conn *systemddbus.Conn) error { + _, err := conn.StopUnitContext(ctx, normalizedName, "replace", c) + return err + })); err != nil { + return fmt.Errorf("stopping systemd unit %q: %w", normalizedName, err) + } + logSystem("Stopped systemd unit %q", normalizedName) + return nil +} + +// Reload reloads a systemd unit. Unit name is automatically normalized. +func (s *systemdConnectionImpl) Reload(ctx context.Context, unit string) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Reloading systemd unit %q", normalizedName) + if err := s.doWithConnection(ctx, doSync(ctx, func(c chan string, conn *systemddbus.Conn) error { + _, err := conn.ReloadUnitContext(ctx, normalizedName, "replace", c) + return err + })); err != nil { + return fmt.Errorf("reloading systemd unit %q: %w", normalizedName, err) + } + logSystem("Reloaded systemd unit %q", normalizedName) + return nil +} + +// Preset resets a systemd unit to its preset state. Unit name is automatically normalized. +// +// Note: This method uses the low-level dbus library directly because +// PresetUnitFiles is not exposed by the coreos/go-systemd library. +func (s *systemdConnectionImpl) Preset(ctx context.Context, unit string) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Presetting systemd unit %q", normalizedName) + dbusConn, err := dbus.SystemBus() + if err != nil { + return fmt.Errorf("failed to connect to system bus: %w", err) + } + + obj := dbusConn.Object("org.freedesktop.systemd1", "/org/freedesktop/systemd1") + + // Call PresetUnitFiles: takes files array, runtime bool, force bool + // Returns changes array and carries_install_info bool + var carriesInstallInfo bool + var changes [][]interface{} + + err = obj.CallWithContext( + ctx, + "org.freedesktop.systemd1.Manager.PresetUnitFiles", + 0, + []string{normalizedName}, + false, false, + ).Store(&carriesInstallInfo, &changes) + if err != nil { + return fmt.Errorf("presetting systemd unit %q: %w", normalizedName, err) + } + + logSystem("Preset systemd unit %q", normalizedName) + return nil +} + +// IsEnabled checks if a systemd unit is enabled. Unit name is automatically normalized. +func (s *systemdConnectionImpl) IsEnabled(ctx context.Context, unit string) (bool, error) { + var enabled bool + err := s.doWithConnection(ctx, func(conn *systemddbus.Conn) error { + normalizedName := NormalizeSystemdUnitNames(unit)[0] + logSystem("Checking if systemd unit %q is enabled", normalizedName) + enabledUnits, err := conn.ListUnitFilesByPatternsContext( + ctx, + []string{"enabled", "enabled-runtime"}, + []string{normalizedName}, + ) + if err != nil { + return fmt.Errorf("checking if unit %q is enabled: %w", normalizedName, err) + } + enabled = len(enabledUnits) > 0 + logSystem("Checked systemd unit %q enabled=%v", normalizedName, enabled) + return nil + }) + return enabled, err +} + +// ListUnits retrieves all currently loaded systemd units and returns them as a map indexed by unit name. +// Only units that are both active and/or enabled will be returned. +func (s *systemdConnectionImpl) ListUnits(ctx context.Context) (map[string]systemddbus.UnitStatus, error) { + var result map[string]systemddbus.UnitStatus + err := s.doWithConnection(ctx, func(conn *systemddbus.Conn) error { + logSystem("Listing systemd units") + units, err := conn.ListUnitsContext(ctx) + if err != nil { + return fmt.Errorf("getting units list: %w", err) + } + result = make(map[string]systemddbus.UnitStatus) + for _, unit := range units { + result[unit.Name] = unit + } + logSystem("Listed %d systemd units", len(result)) + return nil + }) + return result, err +} + +func (s *systemdConnectionImpl) doWithConnection(ctx context.Context, fn func(*systemddbus.Conn) error) error { + conn, err := s.connect(ctx) + if err != nil { + return err + } + return fn(conn) +} + +// doSync wraps a D-Bus unit operation so it blocks until the resulting systemd +// job completes or the context is cancelled. The returned function is compatible +// with doWithConnection. +func doSync(ctx context.Context, fn func(chan string, *systemddbus.Conn) error) func(*systemddbus.Conn) error { + return func(conn *systemddbus.Conn) error { + ch := make(chan string, 1) + if err := fn(ch, conn); err != nil { + return err + } + select { + case result := <-ch: + if result != "done" { + return fmt.Errorf("job result: %s", result) + } + case <-ctx.Done(): + return ctx.Err() + } + return nil + } +} diff --git a/pkg/daemon/systemd_mocks_test.go b/pkg/daemon/systemd_mocks_test.go new file mode 100644 index 0000000000..ad09ef4b4b --- /dev/null +++ b/pkg/daemon/systemd_mocks_test.go @@ -0,0 +1,325 @@ +package daemon + +import ( + "context" + "fmt" + + systemddbus "github.com/coreos/go-systemd/v22/dbus" +) + +const ( + // Systemd unit load states + mockLoadStateLoaded = "loaded" + + // Systemd unit active states + mockActiveStateActive = "active" + mockActiveStateInactive = "inactive" + + // Systemd unit sub states + mockSubStateRunning = "running" + mockSubStateDead = "dead" +) + +// mockSystemdConnection is a mock implementation of SystemdConnection for testing +type mockSystemdConnection struct { + // Unit state - the source of truth for all units + units map[string]*mockUnitState + + // Callback functions - if set, these are called before default behavior + // The callback can perform assertions, modify state, and return errors + // If the callback returns a non-nil error, default behavior is skipped + OnEnableFunc func(ctx context.Context, force bool, units ...string) error + OnDisableFunc func(ctx context.Context, units ...string) error + OnIsEnabledFunc func(ctx context.Context, unit string) (bool, error) + OnTryRestartFunc func(ctx context.Context, unit string) error + OnRestartFunc func(ctx context.Context, unit string) error + OnStartFunc func(ctx context.Context, unit string) error + OnStopFunc func(ctx context.Context, unit string) error + OnReloadFunc func(ctx context.Context, unit string) error + OnPresetFunc func(ctx context.Context, unit string) error + OnReloadDaemonFunc func(ctx context.Context) error + OnListUnitsFunc func(ctx context.Context) (map[string]systemddbus.UnitStatus, error) +} + +type mockUnitState struct { + name string + enabled bool + active bool + loadState string + activeState string + subState string +} + +// newMockSystemdConnection creates a new mock systemd connection with the specified units +func newMockSystemdConnection(units map[string]*mockUnitState) *mockSystemdConnection { + if units == nil { + units = make(map[string]*mockUnitState) + } + return &mockSystemdConnection{ + units: units, + } +} + +// newMockUnitState creates a mock unit with defaults (disabled, inactive) +func newMockUnitState(name string) *mockUnitState { + return &mockUnitState{ + name: name, + enabled: false, + active: false, + loadState: mockLoadStateLoaded, + activeState: mockActiveStateInactive, + subState: mockSubStateDead, + } +} + +// newMockEnabledUnit creates a mock unit that is enabled and active +func newMockEnabledUnit(name string) *mockUnitState { + return &mockUnitState{ + name: name, + enabled: true, + active: true, + loadState: mockLoadStateLoaded, + activeState: mockActiveStateActive, + subState: mockSubStateRunning, + } +} + +func (m *mockSystemdConnection) Enable(ctx context.Context, force bool, units ...string) error { + if m.OnEnableFunc != nil { + if err := m.OnEnableFunc(ctx, force, units...); err != nil { + return err + } + } + + for _, unitName := range units { + if unit, ok := m.units[unitName]; ok { + unit.enabled = true + } else { + // Create unit if it doesn't exist + m.units[unitName] = &mockUnitState{ + name: unitName, + enabled: true, + active: false, + loadState: mockLoadStateLoaded, + activeState: mockActiveStateInactive, + subState: mockSubStateDead, + } + } + } + return nil +} + +func (m *mockSystemdConnection) Disable(ctx context.Context, units ...string) error { + if m.OnDisableFunc != nil { + if err := m.OnDisableFunc(ctx, units...); err != nil { + return err + } + } + + // Default behavior: disable the units + for _, unitName := range units { + if unit, ok := m.units[unitName]; ok { + unit.enabled = false + } + } + return nil +} + +func (m *mockSystemdConnection) IsEnabled(ctx context.Context, unitName string) (bool, error) { + if m.OnIsEnabledFunc != nil { + enabled, err := m.OnIsEnabledFunc(ctx, unitName) + if err != nil { + return false, err + } + return enabled, nil + } + + // Default behavior: return enabled state from units map + if unit, ok := m.units[unitName]; ok { + return unit.enabled, nil + } + return false, fmt.Errorf("unit %q not found", unitName) +} + +func (m *mockSystemdConnection) TryRestart(ctx context.Context, unitName string) error { + if m.OnTryRestartFunc != nil { + if err := m.OnTryRestartFunc(ctx, unitName); err != nil { + return err + } + } + + // Default behavior: only restart if unit is active + if unit, ok := m.units[unitName]; ok && unit.active { + unit.activeState = mockActiveStateActive + unit.subState = mockSubStateRunning + } + return nil +} + +func (m *mockSystemdConnection) Restart(ctx context.Context, unitName string) error { + if m.OnRestartFunc != nil { + if err := m.OnRestartFunc(ctx, unitName); err != nil { + return err + } + } + + // Default behavior: restart the unit + if unit, ok := m.units[unitName]; ok { + unit.active = true + unit.activeState = mockActiveStateActive + unit.subState = mockSubStateRunning + } + return nil +} + +func (m *mockSystemdConnection) Start(ctx context.Context, unitName string) error { + if m.OnStartFunc != nil { + if err := m.OnStartFunc(ctx, unitName); err != nil { + return err + } + } + + // Default behavior: start the unit + if unit, ok := m.units[unitName]; ok { + unit.active = true + unit.activeState = mockActiveStateActive + unit.subState = mockSubStateRunning + } + return nil +} + +func (m *mockSystemdConnection) Stop(ctx context.Context, unitName string) error { + if m.OnStopFunc != nil { + if err := m.OnStopFunc(ctx, unitName); err != nil { + return err + } + } + + // Default behavior: stop the unit + if unit, ok := m.units[unitName]; ok { + unit.active = false + unit.activeState = mockActiveStateInactive + unit.subState = mockSubStateDead + } + return nil +} + +func (m *mockSystemdConnection) Reload(ctx context.Context, unitName string) error { + if m.OnReloadFunc != nil { + if err := m.OnReloadFunc(ctx, unitName); err != nil { + return err + } + } + + // Default behavior: no-op (reload doesn't change state) + return nil +} + +func (m *mockSystemdConnection) Preset(ctx context.Context, unitName string) error { + if m.OnPresetFunc != nil { + if err := m.OnPresetFunc(ctx, unitName); err != nil { + return err + } + } + + // Default behavior: disable the unit (common preset default) + if unit, ok := m.units[unitName]; ok { + unit.enabled = false + } + return nil +} + +func (m *mockSystemdConnection) ReloadDaemon(ctx context.Context) error { + if m.OnReloadDaemonFunc != nil { + if err := m.OnReloadDaemonFunc(ctx); err != nil { + return err + } + } + + // Default behavior: no-op + return nil +} + +func (m *mockSystemdConnection) ListUnits(ctx context.Context) (map[string]systemddbus.UnitStatus, error) { + if m.OnListUnitsFunc != nil { + units, err := m.OnListUnitsFunc(ctx) + if err != nil { + return nil, err + } + return units, nil + } + + // Default behavior: return all units from the state map + result := make(map[string]systemddbus.UnitStatus) + for name, unit := range m.units { + result[name] = systemddbus.UnitStatus{ + Name: unit.name, + LoadState: unit.loadState, + ActiveState: unit.activeState, + SubState: unit.subState, + } + } + return result, nil +} + +func (m *mockSystemdConnection) Close() { + // No-op for mock +} + +// mockSystemdManager is a mock implementation of SystemdManager for testing +type mockSystemdManager struct { + // The connection to return from NewConnection + connection SystemdConnection + + // Callback functions - if set, these are called instead of default behavior + // The callback can perform assertions, modify state, and return errors + OnNewConnectionFunc func(context.Context) (SystemdConnection, error) + OnDoConnectionFunc func(ctx context.Context, fn SystemdManagerDoConnectionFn) error +} + +// newMockSystemdManager creates a new mock systemd manager with a default mock connection +func newMockSystemdManager(units map[string]*mockUnitState) *mockSystemdManager { + return &mockSystemdManager{ + connection: newMockSystemdConnection(units), + } +} + +// newMockSystemdManagerWithConnection creates a new mock systemd manager with a specific connection +func newMockSystemdManagerWithConnection(conn SystemdConnection) *mockSystemdManager { + return &mockSystemdManager{ + connection: conn, + } +} + +// NewConnection returns the configured connection or calls the callback +func (m *mockSystemdManager) NewConnection(_ context.Context) (SystemdConnection, error) { + if m.OnNewConnectionFunc != nil { + conn, err := m.OnNewConnectionFunc(context.Background()) + if err != nil { + return nil, err + } + return conn, nil + } + + // Default behavior: return the configured connection + if m.connection == nil { + return nil, fmt.Errorf("no connection configured") + } + return m.connection, nil +} + +// DoConnection executes a function with a systemd connection, managing the connection lifecycle +func (m *mockSystemdManager) DoConnection(ctx context.Context, fn SystemdManagerDoConnectionFn) error { + if m.OnDoConnectionFunc != nil { + // Callback completely replaces default behavior + return m.OnDoConnectionFunc(ctx, fn) + } + + // Default behavior: create connection, call fn, close connection + conn, err := m.NewConnection(ctx) + if err != nil { + return err + } + defer conn.Close() + return fn(ctx, conn) +} diff --git a/pkg/daemon/systemd_test.go b/pkg/daemon/systemd_test.go new file mode 100644 index 0000000000..0893541600 --- /dev/null +++ b/pkg/daemon/systemd_test.go @@ -0,0 +1,49 @@ +package daemon + +import ( + "reflect" + "testing" +) + +func TestNormalizeUnitNames(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + }{ + { + name: "units without suffix get .service appended", + input: []string{"crio", "kubelet"}, + expected: []string{"crio.service", "kubelet.service"}, + }, + { + name: "units with valid suffixes are preserved", + input: []string{"crio.service", "docker.socket", "fstrim.timer"}, + expected: []string{"crio.service", "docker.socket", "fstrim.timer"}, + }, + { + name: "mixed units", + input: []string{"crio", "docker.socket", "kubelet.service"}, + expected: []string{"crio.service", "docker.socket", "kubelet.service"}, + }, + { + name: "empty slice", + input: []string{}, + expected: []string{}, + }, + { + name: "unit with dots but no valid suffix", + input: []string{"my.custom.name"}, + expected: []string{"my.custom.name.service"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := NormalizeSystemdUnitNames(tt.input...) + if !reflect.DeepEqual(result, tt.expected) { + t.Errorf("NormalizeSystemdUnitNames(%v) = %v, expected %v", tt.input, result, tt.expected) + } + }) + } +} diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index d122d341b0..d877ac5de5 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -84,18 +84,6 @@ func getNodeRef(node *corev1.Node) *corev1.ObjectReference { } } -func restartService(name string) error { - return runCmdSync("systemctl", "restart", name) -} - -func reloadService(name string) error { - return runCmdSync("systemctl", "reload", name) -} - -func reloadDaemon() error { - return runCmdSync("systemctl", constants.DaemonReloadCommand) -} - func (dn *Daemon) finishRebootlessUpdate() error { // Get current state of node, in case of an error reboot state, err := dn.getStateAndConfigs() @@ -163,6 +151,12 @@ func (dn *Daemon) executeReloadServiceNodeDisruptionAction(serviceName string, r // In the end uncordon node to schedule workload. // If at any point an error occurs, we reboot the node so that node has correct configuration. func (dn *Daemon) performPostConfigChangeNodeDisruptionAction(postConfigChangeActions []opv1.NodeDisruptionPolicyStatusAction, configName string) error { + systemdConnection, err := dn.systemdManager.NewConnection(context.Background()) + if err != nil { + return fmt.Errorf("error creating connection to systemd: %w", err) + } + defer systemdConnection.Close() + for _, action := range postConfigChangeActions { // Drain is already completed at this stage and essentially a no-op for this loop, so no need to log that. @@ -218,7 +212,7 @@ func (dn *Daemon) performPostConfigChangeNodeDisruptionAction(postConfigChangeAc case opv1.RestartStatusAction: serviceName := string(action.Restart.ServiceName) - if err := restartService(serviceName); err != nil { + if err := systemdConnection.Restart(context.Background(), serviceName); err != nil { // On RHEL nodes, this service is not available and will error out. // In those cases, directly run the command instead of using the service if serviceName == constants.UpdateCATrustServiceName { @@ -249,19 +243,19 @@ func (dn *Daemon) performPostConfigChangeNodeDisruptionAction(postConfigChangeAc case opv1.ReloadStatusAction: // Execute a generic service reload defined by the action object serviceName := string(action.Reload.ServiceName) - if err := dn.executeReloadServiceNodeDisruptionAction(serviceName, reloadService(serviceName)); err != nil { + if err := dn.executeReloadServiceNodeDisruptionAction(serviceName, systemdConnection.Reload(context.Background(), serviceName)); err != nil { return err } case opv1.SpecialStatusAction: // The special action type requires a CRIO reload - if err := dn.executeReloadServiceNodeDisruptionAction(constants.CRIOServiceName, reloadService(constants.CRIOServiceName)); err != nil { + if err := dn.executeReloadServiceNodeDisruptionAction(constants.CRIOServiceName, systemdConnection.Reload(context.Background(), constants.CRIOServiceName)); err != nil { return err } case opv1.DaemonReloadStatusAction: // Execute daemon-reload - if err := dn.executeReloadServiceNodeDisruptionAction(constants.DaemonReloadCommand, reloadDaemon()); err != nil { + if err := dn.executeReloadServiceNodeDisruptionAction(constants.DaemonReloadCommand, systemdConnection.ReloadDaemon(context.Background())); err != nil { return err } } @@ -282,6 +276,12 @@ func (dn *Daemon) performPostConfigChangeAction(postConfigChangeActions []string return err } + systemdConnection, err := dn.systemdManager.NewConnection(context.Background()) + if err != nil { + return fmt.Errorf("error creating connection to systemd: %w", err) + } + defer systemdConnection.Close() + if ctrlcommon.InSlice(postConfigChangeActionReboot, postConfigChangeActions) { err := upgrademonitor.GenerateAndApplyMachineConfigNodes( &upgrademonitor.Condition{State: mcfgv1.MachineConfigNodeUpdateRebooted, Reason: string(mcfgv1.MachineConfigNodeUpdateRebooted), Message: "Upgrade requires a reboot."}, @@ -322,8 +322,7 @@ func (dn *Daemon) performPostConfigChangeAction(postConfigChangeActions []string if ctrlcommon.InSlice(postConfigChangeActionReloadCrio, postConfigChangeActions) { serviceName := constants.CRIOServiceName - - if err := reloadService(serviceName); err != nil { + if err := systemdConnection.Reload(context.Background(), serviceName); err != nil { if dn.nodeWriter != nil { dn.nodeWriter.Eventf(corev1.EventTypeWarning, "FailedServiceReload", "%s", fmt.Sprintf("Reloading %s service failed. Error: %v", serviceName, err)) } @@ -361,7 +360,7 @@ func (dn *Daemon) performPostConfigChangeAction(postConfigChangeActions []string serviceName := constants.CRIOServiceName - if err := restartService(serviceName); err != nil { + if err := systemdConnection.Restart(context.Background(), serviceName); err != nil { if dn.nodeWriter != nil { dn.nodeWriter.Eventf(corev1.EventTypeWarning, "FailedServiceReload", "%s", fmt.Sprintf("Reloading %s service failed. Error: %v", serviceName, err)) } @@ -2267,6 +2266,12 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e newUnitSet[path] = struct{}{} } + systemdConnection, err := dn.systemdManager.NewConnection(context.Background()) + if err != nil { + return fmt.Errorf("error creating connection to systemd: %w", err) + } + defer systemdConnection.Close() + for _, u := range oldIgnConfig.Systemd.Units { for j := range u.Dropins { path := filepath.Join(pathSystemd, u.Name+".d", u.Dropins[j].Name) @@ -2301,7 +2306,7 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e // look to restore defaults here, so that symlinks are removed first // if the system has the service disabled // writeUnits() will catch units that still have references in other MCs - if err := dn.presetUnit(u); err != nil { + if err := dn.presetUnit(systemdConnection, u); err != nil { klog.Infof("Did not restore preset for %s (may not exist): %s", u.Name, err) } if _, err := os.Stat(noOrigFileStampName(path)); err == nil { @@ -2356,12 +2361,10 @@ func (dn *Daemon) workaroundOcpBugs33694() error { } // enableUnits enables a set of systemd units via systemctl, if any fail all fails. -func (dn *Daemon) enableUnits(units []string) error { - args := append([]string{"enable"}, units...) - stdouterr, err := exec.Command("systemctl", args...).CombinedOutput() - if err != nil { +func (dn *Daemon) enableUnits(systemdConnection SystemdConnection, units []string) error { + if err := systemdConnection.Enable(context.Background(), false, units...); err != nil { if !dn.os.IsLikeTraditionalRHEL7() { - return fmt.Errorf("error enabling units: %s", stdouterr) + return fmt.Errorf("error enabling units: %w", err) } // In RHEL7, the systemd version is too low, so it is unable to handle broken // symlinks during enable. Do a best-effort removal of potentially broken @@ -2373,26 +2376,25 @@ func (dn *Daemon) enableUnits(units []string) error { fi, fiErr := os.Lstat(unitLinkPath) if fiErr != nil { if !os.IsNotExist(fiErr) { - return fmt.Errorf("error trying to enable unit, fallback failed with %s (original error %s)", - fiErr, stdouterr) + return fmt.Errorf("error trying to enable unit, fallback failed with %s (original error %w)", + fiErr, err) } continue } if fi.Mode()&os.ModeSymlink == 0 { - return fmt.Errorf("error trying to enable unit, a non-symlink file exists at %s (original error %s)", - unitLinkPath, stdouterr) + return fmt.Errorf("error trying to enable unit, a non-symlink file exists at %s (original error %w)", + unitLinkPath, err) } if _, evalErr := filepath.EvalSymlinks(unitLinkPath); evalErr != nil { // this is a broken symlink, remove if rmErr := os.Remove(unitLinkPath); rmErr != nil { - return fmt.Errorf("error trying to enable unit, cannot remove broken symlink: %s (original error %s)", - rmErr, stdouterr) + return fmt.Errorf("error trying to enable unit, cannot remove broken symlink: %s (original error %w)", + rmErr, err) } } } - stdouterr, err := exec.Command("systemctl", args...).CombinedOutput() - if err != nil { - return fmt.Errorf("error enabling units: %s", stdouterr) + if err := systemdConnection.Enable(context.Background(), false, units...); err != nil { + return fmt.Errorf("error enabling units: %w", err) } } klog.Infof("Enabled systemd units: %v", units) @@ -2400,22 +2402,18 @@ func (dn *Daemon) enableUnits(units []string) error { } // disableUnits disables a set of systemd units via systemctl, if any fail all fails. -func (dn *Daemon) disableUnits(units []string) error { - args := append([]string{"disable"}, units...) - stdouterr, err := exec.Command("systemctl", args...).CombinedOutput() - if err != nil { - return fmt.Errorf("error disabling unit: %s", stdouterr) +func (dn *Daemon) disableUnits(systemdConnection SystemdConnection, units []string) error { + if err := systemdConnection.Disable(context.Background(), units...); err != nil { + return fmt.Errorf("error disabling unit: %w", err) } klog.Infof("Disabled systemd units %v", units) return nil } // presetUnit resets a systemd unit to its preset via systemctl -func (dn *Daemon) presetUnit(unit ign3types.Unit) error { - args := []string{"preset", unit.Name} - stdouterr, err := exec.Command("systemctl", args...).CombinedOutput() - if err != nil { - return fmt.Errorf("error running preset on unit: %s", stdouterr) +func (dn *Daemon) presetUnit(systemdConnection SystemdConnection, unit ign3types.Unit) error { + if err := systemdConnection.Preset(context.Background(), unit.Name); err != nil { + return fmt.Errorf("error running preset on unit: %w", err) } klog.Infof("Preset systemd unit %q", unit.Name) return nil @@ -2427,13 +2425,19 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error { var disabledUnits []string isCoreOSVariant := dn.os.IsCoreOSVariant() + + systemdConnection, err := dn.systemdManager.NewConnection(context.Background()) + if err != nil { + return fmt.Errorf("error creating connection to systemd: %w", err) + } + defer systemdConnection.Close() + systemdUnits, err := dn.listSystemdUnits() if err != nil { return err } - for _, u := range units { - if err := writeUnit(u, pathSystemd, isCoreOSVariant); err != nil { + if err := writeUnit(systemdConnection, u, pathSystemd, isCoreOSVariant); err != nil { return fmt.Errorf("daemon could not write systemd unit: %w", err) } // if the unit doesn't note if it should be enabled or disabled then @@ -2467,7 +2471,7 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error { klog.Infof("Could not %s unit %q, because it has no contents, skipping", action, u.Name) } } else { - if err := dn.presetUnit(u); err != nil { + if err := dn.presetUnit(systemdConnection, u); err != nil { // Don't fail here, since a unit may have a dropin referencing a nonexisting actual unit klog.Infof("Could not reset unit preset for %s, skipping. (Error msg: %v)", u.Name, err) } @@ -2475,12 +2479,12 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error { } if len(enabledUnits) > 0 { - if err := dn.enableUnits(enabledUnits); err != nil { + if err := dn.enableUnits(systemdConnection, enabledUnits); err != nil { return err } } if len(disabledUnits) > 0 { - if err := dn.disableUnits(disabledUnits); err != nil { + if err := dn.disableUnits(systemdConnection, disabledUnits); err != nil { return err } } @@ -3415,7 +3419,7 @@ func (dn *Daemon) disableRevertSystemdUnit() error { // If we've reached this point, we know that the unit file is still present, // which means that the unit may still be enabled. - if err := dn.disableUnits([]string{runtimeassets.RevertServiceName}); err != nil { + if err := dn.systemdManager.DoConnection(context.Background(), SystemdDisable(runtimeassets.RevertServiceName)); err != nil { return err }