Merge pull request #1647 from endocode/dongsu/fleetd-errcheck-startst…

…op-task

agent,unit: check errors from start/stop/unload-ing unit

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,25 +111,28 @@ 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) {
+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 {

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 {
@@ -114,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 {
@@ -132,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 {

agent/task.go

@@ -104,11 +104,11 @@ 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 }
+		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:

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 {

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 }

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 {

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