diff --git a/agent/agent.go b/agent/agent.go index 1b6ad2074..e580a9ecd 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -98,11 +98,22 @@ func (a *Agent) unloadUnit(unitName string) { a.registry.ClearUnitHeartbeat(unitName) a.cache.dropTargetState(unitName) - a.um.TriggerStop(unitName) + errStop := a.um.TriggerStop(unitName) + if errStop != nil { + log.Warningf("Failed stopping unit(%s): %v", unitName, errStop) + } else { + log.Infof("Stopped unit(%s)", unitName) + } a.uGen.Unsubscribe(unitName) - a.um.Unload(unitName) + // unit should be unloaded and unit file should be removed, only if the unit + // could be successfully stopped. Otherwise the unit could get into a state + // where the unit cannot be stopped via fleet, because the unit file was + // already removed. See also https://github.com/coreos/fleet/issues/1216. + if errStop == nil { + a.um.Unload(unitName) + } } func (a *Agent) startUnit(unitName string) { diff --git a/functional/systemd_test.go b/functional/systemd_test.go index f15313cf0..408543bee 100644 --- a/functional/systemd_test.go +++ b/functional/systemd_test.go @@ -78,14 +78,20 @@ ExecStart=/usr/bin/sleep 3000 t.Error(err.Error()) } - mgr.TriggerStart(name) + err = mgr.TriggerStart(name) + if err != nil { + t.Error(err.Error()) + } err = waitForUnitState(mgr, name, unit.UnitState{"loaded", "active", "running", "", hash, ""}) if err != nil { t.Error(err.Error()) } - mgr.TriggerStop(name) + err = mgr.TriggerStop(name) + if err != nil { + t.Error(err.Error()) + } mgr.Unload(name) diff --git a/systemd/manager.go b/systemd/manager.go index 4ca28f6c7..a7b4dd542 100644 --- a/systemd/manager.go +++ b/systemd/manager.go @@ -123,24 +123,26 @@ func (m *systemdUnitManager) Unload(name string) { // TriggerStart asynchronously starts the unit identified by the given name. // This function does not block for the underlying unit to actually start. -func (m *systemdUnitManager) TriggerStart(name string) { +func (m *systemdUnitManager) TriggerStart(name string) error { jobID, err := m.systemd.StartUnit(name, "replace", nil) - if err == nil { - log.Infof("Triggered systemd unit %s start: job=%d", name, jobID) - } else { + if err != nil { log.Errorf("Failed to trigger systemd unit %s start: %v", name, err) + return err } + log.Infof("Triggered systemd unit %s start: job=%d", name, jobID) + return nil } // TriggerStop asynchronously starts the unit identified by the given name. // This function does not block for the underlying unit to actually stop. -func (m *systemdUnitManager) TriggerStop(name string) { +func (m *systemdUnitManager) TriggerStop(name string) error { jobID, err := m.systemd.StopUnit(name, "replace", nil) - if err == nil { - log.Infof("Triggered systemd unit %s stop: job=%d", name, jobID) - } else { + if err != nil { log.Errorf("Failed to trigger systemd unit %s stop: %v", name, err) + return err } + log.Infof("Triggered systemd unit %s stop: job=%d", name, jobID) + return nil } // GetUnitState generates a UnitState object representing the diff --git a/unit/fake.go b/unit/fake.go index 3d8059476..4a075764f 100644 --- a/unit/fake.go +++ b/unit/fake.go @@ -48,8 +48,8 @@ func (fum *FakeUnitManager) Unload(name string) { delete(fum.u, name) } -func (fum *FakeUnitManager) TriggerStart(string) {} -func (fum *FakeUnitManager) TriggerStop(string) {} +func (fum *FakeUnitManager) TriggerStart(string) error { return nil } +func (fum *FakeUnitManager) TriggerStop(string) error { return nil } func (fum *FakeUnitManager) Units() ([]string, error) { fum.RLock() diff --git a/unit/manager.go b/unit/manager.go index 145b2c072..40918c9da 100644 --- a/unit/manager.go +++ b/unit/manager.go @@ -23,8 +23,8 @@ type UnitManager interface { Unload(string) ReloadUnitFiles() error - TriggerStart(string) - TriggerStop(string) + TriggerStart(string) error + TriggerStop(string) error Units() ([]string, error) GetUnitStates(pkg.Set) (map[string]*UnitState, error)