From 343ae86012e72b37288ac85afc82628cb794ce8a Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 22 Jul 2016 14:08:31 +0200 Subject: [PATCH 1/2] agent,unit: make UnitManager.Unload() return error Now that it's possible to retrieve an error from a.um.TriggerStop(), a.um.Unload() should be able to return an error for consistency. Also the error should be delivered up to mapTaskToFunc() not to be ignored from the task manager. --- agent/agent.go | 7 +++++-- agent/agent_test.go | 5 ++++- agent/task.go | 2 +- systemd/manager.go | 27 ++++++++++++++++++++++----- unit/fake.go | 3 ++- unit/fake_test.go | 5 ++++- unit/manager.go | 2 +- 7 files changed, 39 insertions(+), 12 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index e580a9ecd..310ecd6ca 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -94,7 +94,7 @@ func (a *Agent) loadUnit(u *job.Unit) error { return a.um.Load(u.Name, u.Unit) } -func (a *Agent) unloadUnit(unitName string) { +func (a *Agent) unloadUnit(unitName string) error { a.registry.ClearUnitHeartbeat(unitName) a.cache.dropTargetState(unitName) @@ -111,9 +111,12 @@ func (a *Agent) unloadUnit(unitName string) { // 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. + var errUnload error if errStop == nil { - a.um.Unload(unitName) + errUnload = a.um.Unload(unitName) } + + return errUnload } func (a *Agent) startUnit(unitName string) { diff --git a/agent/agent_test.go b/agent/agent_test.go index 5ea4ba483..9ed18a08f 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -87,7 +87,10 @@ func TestAgentLoadUnloadUnit(t *testing.T) { t.Fatalf("Received unexpected collection of Units: %#v\nExpected: %#v", units, expectUnits) } - a.unloadUnit("foo.service") + err = a.unloadUnit("foo.service") + if err != nil { + t.Fatalf("Failed calling Agent.unloadUnit: %v", err) + } units, err = a.units() if err != nil { diff --git a/agent/task.go b/agent/task.go index 308973860..b4bd82cf0 100644 --- a/agent/task.go +++ b/agent/task.go @@ -104,7 +104,7 @@ func mapTaskToFunc(t task, a *Agent) (fn func() error, err error) { case taskTypeLoadUnit: fn = func() error { return a.loadUnit(t.unit) } case taskTypeUnloadUnit: - fn = func() error { a.unloadUnit(t.unit.Name); return nil } + fn = func() error { return a.unloadUnit(t.unit.Name) } case taskTypeStartUnit: fn = func() error { a.startUnit(t.unit.Name); return nil } case taskTypeStopUnit: diff --git a/systemd/manager.go b/systemd/manager.go index a7b4dd542..ad9a82530 100644 --- a/systemd/manager.go +++ b/systemd/manager.go @@ -114,11 +114,11 @@ func (m *systemdUnitManager) Load(name string, u unit.UnitFile) error { // Unload removes the indicated unit from the filesystem, deletes its // associated Hash from the cache and clears its unit status in systemd -func (m *systemdUnitManager) Unload(name string) { +func (m *systemdUnitManager) Unload(name string) error { m.mutex.Lock() defer m.mutex.Unlock() delete(m.hashes, name) - m.removeUnit(name) + return m.removeUnit(name) } // TriggerStart asynchronously starts the unit identified by the given name. @@ -269,14 +269,31 @@ func (m *systemdUnitManager) writeUnit(name string, contents string) error { return err } -func (m *systemdUnitManager) removeUnit(name string) { +func (m *systemdUnitManager) removeUnit(name string) (err error) { log.Infof("Removing systemd unit %s", name) - m.systemd.DisableUnitFiles([]string{name}, true) - m.systemd.ResetFailedUnit(name) + // both DisableUnitFiles() and ResetFailedUnit() must be followed by + // removing the unit file. Otherwise "systemctl stop fleet" could end up + // hanging forever. + var errf error + func(name string) { + _, errf = m.systemd.DisableUnitFiles([]string{name}, true) + if errf != nil { + err = fmt.Errorf("%v, %v", err, errf) + } + }(name) + + func(name string) { + errf = m.systemd.ResetFailedUnit(name) + if errf != nil { + err = fmt.Errorf("%v, %v", err, errf) + } + }(name) ufPath := m.getUnitFilePath(name) os.Remove(ufPath) + + return err } func (m *systemdUnitManager) getUnitFilePath(name string) string { diff --git a/unit/fake.go b/unit/fake.go index 4a075764f..34bcd5b17 100644 --- a/unit/fake.go +++ b/unit/fake.go @@ -41,11 +41,12 @@ func (fum *FakeUnitManager) ReloadUnitFiles() error { return nil } -func (fum *FakeUnitManager) Unload(name string) { +func (fum *FakeUnitManager) Unload(name string) error { fum.Lock() defer fum.Unlock() delete(fum.u, name) + return nil } func (fum *FakeUnitManager) TriggerStart(string) error { return nil } diff --git a/unit/fake_test.go b/unit/fake_test.go index 60e89ce1e..a0590c66d 100644 --- a/unit/fake_test.go +++ b/unit/fake_test.go @@ -65,7 +65,10 @@ func TestFakeUnitManagerLoadUnload(t *testing.T) { t.Fatalf("Expected UnitState %v, got %v", eus, *us) } - fum.Unload("hello.service") + err = fum.Unload("hello.service") + if err != nil { + t.Fatalf("Expected no error from Unload(), got %v", err) + } units, err = fum.Units() if err != nil { diff --git a/unit/manager.go b/unit/manager.go index 40918c9da..b478005fb 100644 --- a/unit/manager.go +++ b/unit/manager.go @@ -20,7 +20,7 @@ import ( type UnitManager interface { Load(string, UnitFile) error - Unload(string) + Unload(string) error ReloadUnitFiles() error TriggerStart(string) error From 0829c3386439a239337090932ae9d382ed70cc1d Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 22 Jul 2016 14:08:36 +0200 Subject: [PATCH 2/2] agent: check errors from start/stop-ing unit Now that UnitManager.Trigger{Start,Stop}() could return an error, the error should be delivered up to mapTaskToFunc(), not to be ignored by the task manager. Doing that, we can fix a bug in fleet ignoring failed attempt to start/stop a unit. Fixes https://github.com/coreos/fleet/issues/998 --- agent/agent.go | 8 ++++---- agent/agent_test.go | 10 ++++++++-- agent/task.go | 4 ++-- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 310ecd6ca..3ae99db4e 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -119,20 +119,20 @@ func (a *Agent) unloadUnit(unitName string) error { return errUnload } -func (a *Agent) startUnit(unitName string) { +func (a *Agent) startUnit(unitName string) error { a.cache.setTargetState(unitName, job.JobStateLaunched) machID := a.Machine.State().ID a.registry.UnitHeartbeat(unitName, machID, a.ttl) - a.um.TriggerStart(unitName) + return a.um.TriggerStart(unitName) } -func (a *Agent) stopUnit(unitName string) { +func (a *Agent) stopUnit(unitName string) error { a.cache.setTargetState(unitName, job.JobStateLoaded) a.registry.ClearUnitHeartbeat(unitName) - a.um.TriggerStop(unitName) + return a.um.TriggerStop(unitName) } type unitState struct { diff --git a/agent/agent_test.go b/agent/agent_test.go index 9ed18a08f..297cbe02f 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -117,7 +117,10 @@ func TestAgentLoadStartStopUnit(t *testing.T) { t.Fatalf("Failed calling Agent.loadUnit: %v", err) } - a.startUnit("foo.service") + err = a.startUnit("foo.service") + if err != nil { + t.Fatalf("Failed starting unit foo.service: %v", err) + } units, err := a.units() if err != nil { @@ -135,7 +138,10 @@ func TestAgentLoadStartStopUnit(t *testing.T) { t.Fatalf("Received unexpected collection of Units: %#v\nExpected: %#v", units, expectUnits) } - a.stopUnit("foo.service") + err = a.stopUnit("foo.service") + if err != nil { + t.Fatalf("Failed stopping unit foo.service: %v", err) + } units, err = a.units() if err != nil { diff --git a/agent/task.go b/agent/task.go index b4bd82cf0..d293218aa 100644 --- a/agent/task.go +++ b/agent/task.go @@ -106,9 +106,9 @@ func mapTaskToFunc(t task, a *Agent) (fn func() error, err error) { case taskTypeUnloadUnit: fn = func() error { return a.unloadUnit(t.unit.Name) } case taskTypeStartUnit: - fn = func() error { a.startUnit(t.unit.Name); return nil } + fn = func() error { return a.startUnit(t.unit.Name) } case taskTypeStopUnit: - fn = func() error { a.stopUnit(t.unit.Name); return nil } + fn = func() error { return a.stopUnit(t.unit.Name) } case taskTypeReloadUnitFiles: fn = func() error { return a.reloadUnitFiles() } default: