systemd,agent: unload unit only when TriggerStop() runs successfully

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) {

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)
 

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

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()

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)