From 842010c8a8314bed7681627661c2c08949af94d7 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Wed, 11 Mar 2020 09:30:51 -0400 Subject: [PATCH] Fix enabling systemd instantiated services --- config/shared/errors/errors.go | 2 + internal/exec/stages/files/units.go | 126 ++++++++++++++++++++--- internal/exec/stages/files/units_test.go | 76 ++++++++++++++ internal/exec/util/unit.go | 8 +- internal/systemd/systemd.go | 26 +++++ tests/positive/files/units.go | 74 +++++++++++++ 6 files changed, 294 insertions(+), 18 deletions(-) create mode 100644 internal/exec/stages/files/units_test.go create mode 100644 tests/positive/files/units.go diff --git a/config/shared/errors/errors.go b/config/shared/errors/errors.go index 18911082a..78c4441ca 100644 --- a/config/shared/errors/errors.go +++ b/config/shared/errors/errors.go @@ -74,6 +74,8 @@ var ( // Systemd section errors ErrInvalidSystemdExt = errors.New("invalid systemd unit extension") ErrInvalidSystemdDropinExt = errors.New("invalid systemd drop-in extension") + ErrNoSystemdExt = errors.New("no systemd unit extension") + ErrInvalidInstantiatedUnit = errors.New("invalid systemd instantiated unit") // Misc errors ErrInvalidScheme = errors.New("invalid url scheme") diff --git a/internal/exec/stages/files/units.go b/internal/exec/stages/files/units.go index d984c1c3f..00cfce7a1 100644 --- a/internal/exec/stages/files/units.go +++ b/internal/exec/stages/files/units.go @@ -19,35 +19,72 @@ import ( "path/filepath" "strings" + "github.com/coreos/ignition/v2/config/shared/errors" "github.com/coreos/ignition/v2/config/v3_1_experimental/types" "github.com/coreos/ignition/v2/internal/distro" "github.com/coreos/ignition/v2/internal/exec/util" + "github.com/coreos/ignition/v2/internal/systemd" ) +// Preset holds the information about +// a given systemd unit. +type Preset struct { + unit string + enabled bool + instantiatable bool + instances []string +} + +// warnOnOldSystemdVersion checks the version of Systemd +// in a given system and prints a warning if older than 240. +func (s *stage) warnOnOldSystemdVersion() error { + systemdVersion, err := systemd.GetSystemdVersion() + if err != nil { + return err + } + if systemdVersion < 240 { + if err := s.Logger.Warning("The version of systemd (%q) is less than 240. Enabling/disabling instantiated units may not work. See https://github.com/coreos/ignition/issues/586 for more information.", systemdVersion); err != nil { + return err + } + } + return nil +} + // createUnits creates the units listed under systemd.units. func (s *stage) createUnits(config types.Config) error { - enabledOneUnit := false + presets := make(map[string]*Preset) for _, unit := range config.Systemd.Units { if err := s.writeSystemdUnit(unit, false); err != nil { return err } if unit.Enabled != nil { + // identifier keyword is used to distinguish systemd units + // which are either enabled or disabled. Appending + // it to a unitName will avoid overwriting the existing + // unitName's instance if the state of the unit is different. + identifier := "disabled" if *unit.Enabled { - if err := s.Logger.LogOp( - func() error { return s.EnableUnit(unit) }, - "enabling unit %q", unit.Name, - ); err != nil { + identifier = "enabled" + } + if strings.Contains(unit.Name, "@") { + unitName, instance, err := parseInstanceUnit(unit) + if err != nil { return err } + key := fmt.Sprintf("%s-%s", unitName, identifier) + if _, ok := presets[key]; ok { + presets[key].instances = append(presets[key].instances, instance) + } else { + presets[key] = &Preset{unitName, *unit.Enabled, true, []string{instance}} + } } else { - if err := s.Logger.LogOp( - func() error { return s.DisableUnit(unit) }, - "disabling unit %q", unit.Name, - ); err != nil { - return err + key := fmt.Sprintf("%s-%s", unit.Name, identifier) + if _, ok := presets[unit.Name]; !ok { + presets[key] = &Preset{unit.Name, *unit.Enabled, false, []string{}} + } else { + return fmt.Errorf("%q key is already present in the presets map", key) } } - enabledOneUnit = true } if unit.Mask != nil && *unit.Mask { relabelpath := "" @@ -64,10 +101,71 @@ func (s *stage) createUnits(config types.Config) error { s.relabel(relabelpath) } } - // and relabel the preset file itself if we enabled/disabled something - if enabledOneUnit { - s.relabel(util.PresetPath) + // if we have presets then create the systemd preset file. + if len(presets) != 0 { + if err := s.createSystemdPresetFile(presets); err != nil { + return err + } + } + + return nil +} + +// parseInstanceUnit extracts the name and a corresponding instance +// for a given instantiated unit. +// e.g: echo@bar.service ==> unitName=echo@.service & instance=bar +func parseInstanceUnit(unit types.Unit) (string, string, error) { + at := strings.Index(unit.Name, "@") + if at == -1 { + return "", "", errors.ErrInvalidInstantiatedUnit + } + dot := strings.LastIndex(unit.Name, ".") + if dot == -1 { + return "", "", errors.ErrNoSystemdExt + } + instance := unit.Name[at+1 : dot] + serviceInstance := unit.Name[0:at+1] + unit.Name[dot:len(unit.Name)] + return serviceInstance, instance, nil +} + +// createSystemdPresetFile creates the presetfile for enabled/disabled +// systemd units. +func (s *stage) createSystemdPresetFile(presets map[string]*Preset) error { + hasInstanceUnit := false + for _, value := range presets { + unitString := value.unit + if value.instantiatable { + hasInstanceUnit = true + // Let's say we have two instantiated enabled units listed under + // the systemd units i.e. echo@foo.service, echo@bar.service + // then the unitString will look like "echo@.service foo bar" + unitString = fmt.Sprintf("%s %s", unitString, strings.Join(value.instances, " ")) + } + if value.enabled { + if err := s.Logger.LogOp( + func() error { return s.EnableUnit(unitString) }, + "setting preset to enabled for %q", unitString, + ); err != nil { + return err + } + } else { + if err := s.Logger.LogOp( + func() error { return s.DisableUnit(unitString) }, + "setting preset to disabled for %q", unitString, + ); err != nil { + return err + } + } + } + // Print the warning if there's an instantiated unit present under + // the systemd units and the version of systemd in a given system + // is older than 240. + if hasInstanceUnit { + if err := s.warnOnOldSystemdVersion(); err != nil { + return err + } } + s.relabel(util.PresetPath) return nil } diff --git a/internal/exec/stages/files/units_test.go b/internal/exec/stages/files/units_test.go new file mode 100644 index 000000000..57618f86a --- /dev/null +++ b/internal/exec/stages/files/units_test.go @@ -0,0 +1,76 @@ +// Copyright 2020 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package files + +import ( + "reflect" + "testing" + + "github.com/coreos/ignition/v2/config/shared/errors" + "github.com/coreos/ignition/v2/config/v3_1_experimental/types" +) + +func TestParseInstanceUnit(t *testing.T) { + type in struct { + unit types.Unit + } + type out struct { + unitName string + instance string + parseErr error + } + tests := []struct { + in in + out out + }{ + {in: in{types.Unit{Name: "echo@bar.service"}}, + out: out{unitName: "echo@.service", instance: "bar", + parseErr: nil}, + }, + + {in: in{types.Unit{Name: "echo@foo.service"}}, + out: out{unitName: "echo@.service", instance: "foo", + parseErr: nil}, + }, + {in: in{types.Unit{Name: "echo.service"}}, + out: out{unitName: "", instance: "", + parseErr: errors.ErrInvalidInstantiatedUnit}, + }, + {in: in{types.Unit{Name: "echo@fooservice"}}, + out: out{unitName: "", instance: "", + parseErr: errors.ErrNoSystemdExt}, + }, + {in: in{types.Unit{Name: "echo@.service"}}, + out: out{unitName: "echo@.service", instance: "", + parseErr: nil}, + }, + {in: in{types.Unit{Name: "postgresql@9.3-main.service"}}, + out: out{unitName: "postgresql@.service", instance: "9.3-main", + parseErr: nil}, + }, + } + for i, test := range tests { + unitName, instance, err := parseInstanceUnit(test.in.unit) + if test.out.parseErr != err { + t.Errorf("#%d: bad error: want %v, got %v", i, test.out.parseErr, err) + } + if !reflect.DeepEqual(test.out.unitName, unitName) { + t.Errorf("#%d: bad unitName: want %v, got %v", i, test.out.unitName, unitName) + } + if !reflect.DeepEqual(test.out.instance, instance) { + t.Errorf("#%d: bad instance: want %v, got %v", i, test.out.instance, instance) + } + } +} diff --git a/internal/exec/util/unit.go b/internal/exec/util/unit.go index e9c7bc73d..b45a3038e 100644 --- a/internal/exec/util/unit.go +++ b/internal/exec/util/unit.go @@ -110,8 +110,8 @@ func (ut Util) MaskUnit(unit types.Unit) (string, error) { return filepath.Join("/", SystemdUnitsPath(), unit.Name), nil } -func (ut Util) EnableUnit(unit types.Unit) error { - return ut.appendLineToPreset(fmt.Sprintf("enable %s", unit.Name)) +func (ut Util) EnableUnit(enabledUnit string) error { + return ut.appendLineToPreset(fmt.Sprintf("enable %s", enabledUnit)) } // presets link in /etc, which doesn't make sense for runtime units @@ -145,8 +145,8 @@ func (ut Util) EnableRuntimeUnit(unit types.Unit, target string) error { return ut.WriteLink(link) } -func (ut Util) DisableUnit(unit types.Unit) error { - return ut.appendLineToPreset(fmt.Sprintf("disable %s", unit.Name)) +func (ut Util) DisableUnit(disabledUnit string) error { + return ut.appendLineToPreset(fmt.Sprintf("disable %s", disabledUnit)) } func (ut Util) appendLineToPreset(data string) error { diff --git a/internal/systemd/systemd.go b/internal/systemd/systemd.go index 39d457a2d..864d8262b 100644 --- a/internal/systemd/systemd.go +++ b/internal/systemd/systemd.go @@ -16,6 +16,8 @@ package systemd import ( "fmt" + "regexp" + "strconv" "github.com/coreos/go-systemd/dbus" "github.com/coreos/go-systemd/unit" @@ -48,3 +50,27 @@ func WaitOnDevices(devs []string, stage string) error { return nil } + +// GetSystemdVersion fetches the version of Systemd +// in a given system. +func GetSystemdVersion() (uint, error) { + conn, err := dbus.NewSystemdConnection() + if err != nil { + return 0, err + } + version, err := conn.GetManagerProperty("Version") + if err != nil { + return 0, err + } + // Handle different systemd versioning schemes that are being returned. + // for e.g: + // - Fedora 31: `"v243.5-1.fc31"` + // - RHEL 8: `"239"` + re := regexp.MustCompile(`\d+`) + systemdVersion := re.FindString(version) + value, err := strconv.Atoi(systemdVersion) + if err != nil { + return 0, err + } + return uint(value), nil +} diff --git a/tests/positive/files/units.go b/tests/positive/files/units.go new file mode 100644 index 000000000..e8b5d0db2 --- /dev/null +++ b/tests/positive/files/units.go @@ -0,0 +1,74 @@ +// Copyright 2020 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package files + +import ( + "github.com/coreos/ignition/v2/tests/register" + "github.com/coreos/ignition/v2/tests/types" +) + +func init() { + register.Register(register.PositiveTest, CreateInstantiatedService()) +} + +func CreateInstantiatedService() types.Test { + name := "instantiated.unit.create" + in := types.GetBaseDisk() + out := types.GetBaseDisk() + config := `{ + "ignition": { "version": "$version" }, + "systemd": { + "units": [ + { + "name": "echo@.service" + "contents": "[Unit]\nDescription=f\n[Service]\nType=oneshot\nExecStart=/bin/echo %i\nRemainAfterExit=yes\n[Install]\nWantedBy=multi-user.target\n", + }, + { + "enabled": true, + "name": "echo@bar.service" + }, + { + "enabled": true, + "name": "echo@foo.service" + }, + ] + } + }` + configMinVersion := "3.0.0" + out[0].Partitions.AddFiles("ROOT", []types.File{ + { + Node: types.Node{ + Name: "echo@.service", + Directory: "etc/systemd/system", + }, + Contents: "[Unit]\nDescription=f\n[Service]\nType=oneshot\nExecStart=/bin/echo %i\nRemainAfterExit=yes\n[Install]\nWantedBy=multi-user.target\n", + }, + { + Node: types.Node{ + Name: "20-ignition.preset", + Directory: "etc/systemd/system-preset", + }, + Contents: "enable echo@.service bar foo\n", + }, + }) + + return types.Test{ + Name: name, + In: in, + Out: out, + Config: config, + ConfigMinVersion: configMinVersion, + } +}