From 9604dc6091935428af8126f2b4b87d0526d7dc21 Mon Sep 17 00:00:00 2001
From: Dongsu Park <dongsu@endocode.com>
Date: Thu, 28 Jul 2016 15:37:21 +0200
Subject: [PATCH] fleetctl: make load & start run only for necessary units

fleetctl load and start have been working for all given units, no matter
whether each unit's target state is more activated than the current state
or not. That means, for example, fleetctl load on activated units
results in stopping the units, which could be unexpected for users.

To fix that, check that each unit really needs to be waited, by running
unitToBeChanged().

Fixes https://github.com/coreos/fleet/issues/1428
---
 fleetctl/fleetctl.go | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go
index 108e29640..7c1ff5e57 100644
--- a/fleetctl/fleetctl.go
+++ b/fleetctl/fleetctl.go
@@ -890,18 +890,26 @@ func isLocalUnitDifferent(cCmd *cobra.Command, file string, su *schema.Unit, fat
 
 func lazyLoadUnits(args []string) ([]*schema.Unit, error) {
 	units := make([]string, 0, len(args))
+	js := job.JobStateLoaded
 	for _, j := range args {
-		units = append(units, unitNameMangle(j))
+		uName := unitNameMangle(j)
+		if unitToBeChanged(uName, js) {
+			units = append(units, uName)
+		}
 	}
-	return setTargetStateOfUnits(units, job.JobStateLoaded)
+	return setTargetStateOfUnits(units, js)
 }
 
 func lazyStartUnits(args []string) ([]*schema.Unit, error) {
 	units := make([]string, 0, len(args))
+	js := job.JobStateLaunched
 	for _, j := range args {
-		units = append(units, unitNameMangle(j))
+		uName := unitNameMangle(j)
+		if unitToBeChanged(uName, js) {
+			units = append(units, uName)
+		}
 	}
-	return setTargetStateOfUnits(units, job.JobStateLaunched)
+	return setTargetStateOfUnits(units, js)
 }
 
 // setTargetStateOfUnits ensures that the target state for the given Units is set
@@ -1158,6 +1166,31 @@ func machineState(machID string) (*machine.MachineState, error) {
 	return nil, nil
 }
 
+// unitToBeChanged returns true if state of a given unit is a more activated
+// state than the current state. A wrapper of jsToBeChanged.
+func unitToBeChanged(name string, js job.JobState) bool {
+	var state string
+
+	u, err := cAPI.Unit(name)
+	if err != nil {
+		log.Warningf("Error retrieving Unit(%s) from Registry: %v", name, err)
+		return false
+	}
+	if u == nil {
+		log.Warningf("Unit %s not found", name)
+		return false
+	}
+
+	// If this is a global unit, CurrentState will never be set. Instead, wait for DesiredState.
+	if suToGlobal(*u) {
+		state = u.DesiredState
+	} else {
+		state = u.CurrentState
+	}
+
+	return jsToBeChanged(job.JobState(state), js)
+}
+
 // jsToBeChanged returns true if the target state is a more activated state
 // than the current state. For example, it returns true if it's a transition
 // from inactive to launched, but false for other way around.