Repository: coreos/fleet
Pull request #1624 (2016-07-06 19:13:03)

fleetctl: status and restart commands with global units
This PR is _yet another_ revision of a long-running issue, `fleetctl restart` and `status` with global units.
- First allow `"fleetctl status"` to run with global units. For the purpose, implement a helper `cmdGlobalMachineState()` that filters a list of machines only with unique entries.
- Introduce a new command `"fleetctl restart"`. This part is originally taken from https://github.com/coreos/fleet/pull/1238 written by "Kyle Boorky kboorky@turbine.com", committed by @hhoover . 
- Introduce unit test and functional test for `fleetctl restart`.
- Add new tests for global tests in `TestScheduleGlobalUnits()`.

This PR is not about changing API or metadata format in etcd registry. It's not introducing any new mechanism, because I don't think I can afford changing such design at the moment. It's rather a compromise between practical requirements and the current design of fleet. Though we still need to think about how to decrease number of ssh connections when restarting global units, as suggested by @kayrus. See also https://github.com/coreos/fleet/issues/1559.

Fixes https://github.com/coreos/fleet/issues/975


simonvanderveldt: @dongsupark have you been using this since you created this PR? How's it holding up?
This will only allow for restarting existing units and doesn't enable updating an existing global unit one by one, right?

We've been doing something similar outside of fleet using SSH connections and calling `systemctl`, but had some issues with fleet getting in a broken state when updating global units, which seemed to be caused by the fact that we weren't updating the desired state of the units in fleet (ie we submitted a new one using fleet and then started it using `systemctl` without doing a `fleetctl start`).

Is anyone planning to review and merge this in the near future?
dongsupark: @simonvanderveldt 
I'm just waiting for anyone to review this PR. And I thought it wouldn't be that urgent issue.
I'm also aware that in the past not everyone was happy with supporting the restart command. So I wouldn't just merge this PR, unless anyone could review it.

Patch:
diff --git a/agent/state.go b/agent/state.go
index 7562ab19..c254147f 100644
--- a/agent/state.go
+++ b/agent/state.go
@@ -39,42 +39,31 @@ func (as *AgentState) unitScheduled(name string) bool {
 	return as.Units[name] != nil
 }
 
-func hasStringInSlice(inSlice []string, unitName string) bool {
-	for _, elem := range inSlice {
-		if globMatches(elem, unitName) {
-			return true
-		}
-	}
-	return false
-}
-
 // HasConflict determines whether there are any known conflicts with the given Unit
-func (as *AgentState) HasConflict(pUnitName string, pConflicts []string) (bool, []string) {
-	found := false
-	conflicts := []string{}
-
+func (as *AgentState) HasConflict(pUnitName string, pConflicts []string) (found bool, conflict string) {
 	for _, eUnit := range as.Units {
 		if pUnitName == eUnit.Name {
 			continue
 		}
 
-		if hasStringInSlice(eUnit.Conflicts(), pUnitName) {
-			conflicts = append(conflicts, pUnitName)
-			found = true
-			break
-		}
-		if hasStringInSlice(pConflicts, eUnit.Name) {
-			conflicts = append(conflicts, eUnit.Name)
-			found = true
-			break
+		for _, pConflict := range pConflicts {
+			if globMatches(pConflict, eUnit.Name) {
+				found = true
+				conflict = eUnit.Name
+				return
+			}
 		}
-	}
 
-	if !found {
-		return false, []string{}
+		for _, eConflict := range eUnit.Conflicts() {
+			if globMatches(eConflict, pUnitName) {
+				found = true
+				conflict = eUnit.Name
+				return
+			}
+		}
 	}
 
-	return true, conflicts
+	return
 }
 
 // hasReplace determines whether there are any known replaces with the given Unit
diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go
index 30cd7beb..48150e08 100644
--- a/fleetctl/fleetctl.go
+++ b/fleetctl/fleetctl.go
@@ -1246,3 +1246,35 @@ func waitForState(stateCheckFunc func() error) (time.Duration, error) {
 		}
 	}
 }
+
+// cmdGlobalMachineState runs a specific fleetctl command on each target machine
+// where global units are started. To avoid unnecessary ssh connections being
+// alive, it filters out the list of machines as much as possible.
+func cmdGlobalMachineState(cCmd *cobra.Command, globalUnits []schema.Unit) (err error) {
+	cmd := cCmd.Name()
+	mapUNs := map[string]string{}
+	for _, unit := range globalUnits {
+		m := cachedMachineState(unit.MachineID)
+		if m == nil || m.ID == "" || m.PublicIP == "" {
+			continue
+		}
+		mapUNs[m.ID] = unit.Name
+	}
+
+	// create a list of unique unit names
+	resultIDs := map[string]string{}
+	for id, name := range mapUNs {
+		resultIDs[id] = name
+	}
+
+	for id, name := range resultIDs {
+		// run a correspondent systemctl command
+		if exitVal := runCommand(cCmd, id, "systemctl", cmd, name); exitVal != 0 {
+			err = fmt.Errorf("Error running systemctl %s. machine id=%v, unit name=%s",
+				cmd, id, name)
+			break
+		}
+	}
+
+	return err
+}
diff --git a/fleetctl/restart.go b/fleetctl/restart.go
new file mode 100644
index 00000000..df180963
--- /dev/null
+++ b/fleetctl/restart.go
@@ -0,0 +1,125 @@
+// Copyright 2016 The fleet Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"os"
+
+	"github.com/spf13/cobra"
+
+	"github.com/coreos/fleet/job"
+	"github.com/coreos/fleet/log"
+	"github.com/coreos/fleet/schema"
+)
+
+var cmdRestart = &cobra.Command{
+	Use:   "restart [--block-attempts=N] UNIT...",
+	Short: "Instruct systemd to rolling restart one or more units in the cluster.",
+	Long: `Restarts one or more units in the cluster. If they are stopped it starts them.
+
+Instructs systemd on the host machine to stop then start the unit, deferring to systemd
+completely for any custom restart directives (i.e. ExecStop options in the unit
+file).
+
+For units which are not global, restart operations are performed synchronously,
+which means fleetctl will block until it detects that the unit(s) have
+transitioned to a stopped state and then back to a started state. This behaviour can be configured with the
+respective --block-attempts options. Restart operations on global
+units are always non-blocking.
+
+Restart a single unit:
+	fleetctl restart foo.service
+
+Restart an entire directory of units with glob matching, without waiting:
+	fleetctl restart myservice/*`,
+	Run: runWrapper(runRestartUnit),
+}
+
+func init() {
+	cmdFleet.AddCommand(cmdRestart)
+
+	cmdRestart.Flags().IntVar(&sharedFlags.BlockAttempts, "block-attempts", 0, "Wait until the units are stopped/started, performing up to N attempts before giving up. A value of 0 indicates no limit. Does not apply to global units.")
+}
+
+func runRestartUnit(cCmd *cobra.Command, args []string) (exit int) {
+	if len(args) == 0 {
+		stderr("No units given")
+		return 0
+	}
+	units, err := findUnits(args)
+	if err != nil {
+		stderr("%v", err)
+		return 1
+	}
+
+	if err := lazyCreateUnits(cCmd, args); err != nil {
+		stderr("Error creating units: %v", err)
+		return 1
+	}
+
+	globalUnits := make([]schema.Unit, 0)
+	for _, unit := range units {
+		if suToGlobal(unit) {
+			globalUnits = append(globalUnits, unit)
+			continue
+		}
+		if job.JobState(unit.CurrentState) == job.JobStateInactive {
+			stderr("Unable to restart unit %s in state %s", unit.Name, job.JobStateInactive)
+			continue
+		} else if job.JobState(unit.CurrentState) == job.JobStateLoaded {
+			log.Infof("Unit(%s) already %s, starting.", unit.Name, job.JobStateLoaded)
+
+			exit = setUnitStateAndWait(unit, job.JobStateLaunched, getBlockAttempts(cCmd))
+			if exit == 1 {
+				return exit
+			}
+			continue
+		} else {
+			//stop and start it
+			exit = setUnitStateAndWait(unit, job.JobStateLoaded, getBlockAttempts(cCmd))
+			if exit == 1 {
+				return exit
+			}
+			exit = setUnitStateAndWait(unit, job.JobStateLaunched, getBlockAttempts(cCmd))
+			if exit == 1 {
+				return exit
+			}
+		}
+		log.Infof("Unit(%s) was restarted.", unit.Name)
+	}
+
+	if err := cmdGlobalMachineState(cCmd, globalUnits); err != nil {
+		stderr("Error restarting global units %v err:%v", globalUnits, err)
+		return 1
+	}
+
+	return
+}
+
+func setUnitStateAndWait(unit schema.Unit, targetState job.JobState, blockAttempts int) (exit int) {
+	err := cAPI.SetUnitTargetState(unit.Name, string(targetState))
+	if err != nil {
+		stderr("Error setting target state for unit %s: %v", unit.Name, err)
+		return 1
+	}
+
+	err = tryWaitForUnitStates([]string{unit.Name}, "restart", job.JobStateLaunched, blockAttempts, os.Stdout)
+	if err != nil {
+		stderr("Error waiting for unit states, exit status: %v", err)
+		return 1
+	}
+
+	return 0
+}
diff --git a/fleetctl/status.go b/fleetctl/status.go
index 20f15864..c24e299c 100644
--- a/fleetctl/status.go
+++ b/fleetctl/status.go
@@ -20,6 +20,7 @@ import (
 	"github.com/spf13/cobra"
 
 	"github.com/coreos/fleet/job"
+	"github.com/coreos/fleet/schema"
 )
 
 var cmdStatus = &cobra.Command{
@@ -46,6 +47,7 @@ func init() {
 }
 
 func runStatusUnit(cCmd *cobra.Command, args []string) (exit int) {
+	globalUnits := make([]schema.Unit, 0)
 	for i, arg := range args {
 		name := unitNameMangle(arg)
 		unit, err := cAPI.Unit(name)
@@ -58,8 +60,8 @@ func runStatusUnit(cCmd *cobra.Command, args []string) (exit int) {
 			stderr("Unit %s does not exist.", name)
 			return 1
 		} else if suToGlobal(*unit) {
-			stderr("Unable to determine status of global unit %s.", unit.Name)
-			return 1
+			globalUnits = append(globalUnits, *unit)
+			continue
 		} else if job.JobState(unit.CurrentState) == job.JobStateInactive {
 			stderr("Unit %s does not appear to be loaded.", unit.Name)
 			return 1
@@ -76,5 +78,10 @@ func runStatusUnit(cCmd *cobra.Command, args []string) (exit int) {
 		}
 	}
 
+	if err := cmdGlobalMachineState(cCmd, globalUnits); err != nil {
+		stderr("Error retrieving machine state for global units: %v", err)
+		return 1
+	}
+
 	return
 }
diff --git a/functional/fixtures/units/conflict.multiple.0.service b/functional/fixtures/units/conflict.multiple.0.service
deleted file mode 100644
index 4b30e8fc..00000000
--- a/functional/fixtures/units/conflict.multiple.0.service
+++ /dev/null
@@ -1,8 +0,0 @@
-[Unit]
-Description=Test Unit
-
-[Service]
-ExecStart=/bin/bash -c "while true; do echo Hello, World!; sleep 1; done"
-
-[X-Fleet]
-Conflicts=conflict.2.service conflict.1.service conflict.0.service
diff --git a/functional/fixtures/units/conflict.multiple.1.service b/functional/fixtures/units/conflict.multiple.1.service
deleted file mode 100644
index 4b30e8fc..00000000
--- a/functional/fixtures/units/conflict.multiple.1.service
+++ /dev/null
@@ -1,8 +0,0 @@
-[Unit]
-Description=Test Unit
-
-[Service]
-ExecStart=/bin/bash -c "while true; do echo Hello, World!; sleep 1; done"
-
-[X-Fleet]
-Conflicts=conflict.2.service conflict.1.service conflict.0.service
diff --git a/functional/fixtures/units/conflict.multiple.2.service b/functional/fixtures/units/conflict.multiple.2.service
deleted file mode 100644
index 5a93342d..00000000
--- a/functional/fixtures/units/conflict.multiple.2.service
+++ /dev/null
@@ -1,10 +0,0 @@
-[Unit]
-Description=Test Unit
-
-[Service]
-ExecStart=/bin/bash -c "while true; do echo Hello, World!; sleep 1; done"
-
-[X-Fleet]
-Conflicts=conflict.2.service
-Conflicts=conflict.1.service
-Conflicts=conflict.0.service
diff --git a/job/job.go b/job/job.go
index a8dbf8cb..1ed001f1 100644
--- a/job/job.go
+++ b/job/job.go
@@ -220,13 +220,8 @@ func (j *Job) ValidateRequirements() error {
 // machine as this Job.
 func (j *Job) Conflicts() []string {
 	conflicts := make([]string, 0)
-
-	ldConflicts := splitCombine(j.requirements()[deprecatedXPrefix+fleetConflicts])
-	conflicts = append(conflicts, ldConflicts...)
-
-	dConflicts := splitCombine(j.requirements()[fleetConflicts])
-	conflicts = append(conflicts, dConflicts...)
-
+	conflicts = append(conflicts, j.requirements()[deprecatedXPrefix+fleetConflicts]...)
+	conflicts = append(conflicts, j.requirements()[fleetConflicts]...)
 	return conflicts
 }
 
@@ -337,14 +332,3 @@ func isTruthyValue(s string) bool {
 	chl := strings.ToLower(s)
 	return chl == "true" || chl == "yes" || chl == "1" || chl == "on" || chl == "t"
 }
-
-// splitCombine retrieves each word from an input string slice, to put each
-// one again into a single slice.
-func splitCombine(inStrs []string) []string {
-	outStrs := make([]string, 0)
-	for _, str := range inStrs {
-		inStrs := strings.Fields(str)
-		outStrs = append(outStrs, inStrs...)
-	}
-	return outStrs
-}


Test patch:
diff --git a/agent/reconcile_test.go b/agent/reconcile_test.go
index 20a0b653..8ee79ac4 100644
--- a/agent/reconcile_test.go
+++ b/agent/reconcile_test.go
@@ -388,30 +388,6 @@ func TestAbleToRun(t *testing.T) {
 			want: job.JobActionUnschedule,
 		},
 
-		// conflicts found
-		{
-			dState: &AgentState{
-				MState: &machine.MachineState{ID: "123"},
-				Units: map[string]*job.Unit{
-					"ping.service": &job.Unit{Name: "ping.service"},
-				},
-			},
-			job:  newTestJobWithXFleetValues(t, "Conflicts=pong.service\nConflicts=ping.service"),
-			want: job.JobActionUnschedule,
-		},
-
-		// conflicts found
-		{
-			dState: &AgentState{
-				MState: &machine.MachineState{ID: "123"},
-				Units: map[string]*job.Unit{
-					"ping.service": &job.Unit{Name: "ping.service"},
-				},
-			},
-			job:  newTestJobWithXFleetValues(t, "Conflicts=pong.service ping.service"),
-			want: job.JobActionUnschedule,
-		},
-
 		// no replaces found
 		{
 			dState: &AgentState{
diff --git a/agent/state_test.go b/agent/state_test.go
index f4be7a7d..e259949d 100644
--- a/agent/state_test.go
+++ b/agent/state_test.go
@@ -82,86 +82,6 @@ func TestHasConflicts(t *testing.T) {
 			want:     true,
 			conflict: "bar.service",
 		},
-
-		// existing job has conflict with new job,
-		// one of multiple conflicts defined in a single line
-		{
-			cState: &AgentState{
-				MState: &machine.MachineState{ID: "XXX"},
-				Units: map[string]*job.Unit{
-					"bar.service": &job.Unit{
-						Name: "bar.service",
-						Unit: fleetUnit(t, "Conflicts=foo.service bar.service"),
-					},
-				},
-			},
-			job: &job.Job{
-				Name: "foo.service",
-				Unit: unit.UnitFile{},
-			},
-			want:     true,
-			conflict: "bar.service",
-		},
-
-		// existing job has conflict with new job,
-		// one of multiple conflicts over multiple lines
-		{
-			cState: &AgentState{
-				MState: &machine.MachineState{ID: "XXX"},
-				Units: map[string]*job.Unit{
-					"bar.service": &job.Unit{
-						Name: "bar.service",
-						Unit: fleetUnit(t, "Conflicts=foo.service\nConflicts=bar.service"),
-					},
-				},
-			},
-			job: &job.Job{
-				Name: "foo.service",
-				Unit: unit.UnitFile{},
-			},
-			want:     true,
-			conflict: "bar.service",
-		},
-
-		// new job has conflict with existing job,
-		// one of multiple conflicts defined in a single line
-		{
-			cState: &AgentState{
-				MState: &machine.MachineState{ID: "XXX"},
-				Units: map[string]*job.Unit{
-					"bar.service": &job.Unit{
-						Name: "bar.service",
-						Unit: unit.UnitFile{},
-					},
-				},
-			},
-			job: &job.Job{
-				Name: "foo.service",
-				Unit: fleetUnit(t, "Conflicts=foo.service bar.service"),
-			},
-			want:     true,
-			conflict: "bar.service",
-		},
-
-		// new job has conflict with existing job,
-		// one of multiple conflicts over multiple lines
-		{
-			cState: &AgentState{
-				MState: &machine.MachineState{ID: "XXX"},
-				Units: map[string]*job.Unit{
-					"bar.service": &job.Unit{
-						Name: "bar.service",
-						Unit: unit.UnitFile{},
-					},
-				},
-			},
-			job: &job.Job{
-				Name: "foo.service",
-				Unit: fleetUnit(t, "Conflicts=foo.service\nConflicts=bar.service"),
-			},
-			want:     true,
-			conflict: "bar.service",
-		},
 	}
 
 	for i, tt := range tests {
diff --git a/fleetctl/restart_test.go b/fleetctl/restart_test.go
new file mode 100644
index 00000000..cffb2ba2
--- /dev/null
+++ b/fleetctl/restart_test.go
@@ -0,0 +1,136 @@
+// Copyright 2016 CoreOS, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"fmt"
+	"sync"
+	"testing"
+
+	"github.com/coreos/fleet/schema"
+)
+
+func checkRestartUnitState(unit schema.Unit, restartRet int, errchan chan error) {
+	if restartRet != 0 && unit.DesiredState != "" {
+		// if the whole restart operation failed, then no unit
+		// should have a DesiredState set
+		errchan <- fmt.Errorf("Error: Unit(%s) DesiredState was set to (%s)", unit.Name, unit.DesiredState)
+	}
+}
+
+func doRestartUnits(t *testing.T, r commandTestResults, errchan chan error) {
+	sharedFlags.NoBlock = true
+	exit := runRestartUnit(cmdRestart, r.units)
+	if exit != r.expectedExit {
+		errchan <- fmt.Errorf("%s: expected exit code %d but received %d", r.description, r.expectedExit, exit)
+		return
+	}
+
+	real_units, err := findUnits(r.units)
+	if err != nil {
+		errchan <- err
+		return
+	}
+
+	for _, v := range real_units {
+		checkRestartUnitState(v, r.expectedExit, errchan)
+	}
+}
+
+func runRestartUnits(t *testing.T, unitPrefix string, results []commandTestResults, template bool) {
+	unitsCount := 0
+	for _, r := range results {
+		var wg_res sync.WaitGroup
+
+		if !template {
+			unitsCount = len(r.units)
+		}
+
+		cAPI = newFakeRegistryForCommands(unitPrefix, unitsCount, template)
+
+		errchan_res := make(chan error)
+		wg_res.Add(1)
+		go func() {
+			defer wg_res.Done()
+			doRestartUnits(t, r, errchan_res)
+		}()
+
+		go func() {
+			wg_res.Wait()
+			close(errchan_res)
+		}()
+
+		for err := range errchan_res {
+			t.Errorf("%v", err)
+		}
+	}
+}
+
+func TestRunRestartUnits(t *testing.T) {
+	unitPrefix := "restart"
+	oldNoBlock := sharedFlags.NoBlock
+	defer func() {
+		sharedFlags.NoBlock = oldNoBlock
+	}()
+
+	results := []commandTestResults{
+		{
+			"restart available units",
+			[]string{"restart1", "restart2", "restart3", "restart4", "restart5", "restart6"},
+			0,
+		},
+		{
+			"restart non-available units",
+			[]string{"y1", "y2"},
+			1,
+		},
+		{
+			"restart available and non-available units",
+			[]string{"y1", "y2", "y3", "y4", "restart1", "restart2", "restart3", "restart4", "restart5", "restart6", "y0"},
+			1,
+		},
+		{
+			"restart a unit from a non-available template",
+			[]string{"foo-template@1"},
+			1,
+		},
+		{
+			"restart null input",
+			[]string{},
+			0,
+		},
+	}
+
+	templateResults := []commandTestResults{
+		{
+			"restart a unit from a non-available template",
+			[]string{"restart-foo@1"},
+			1,
+		},
+		{
+			"restart units from an available template",
+			[]string{"restart@1", "restart@100", "restart@1000"},
+			0,
+		},
+		{
+			"restart same unit from an available template",
+			[]string{"restart@1", "restart@1", "restart@1"},
+			0,
+		},
+	}
+
+	runRestartUnits(t, unitPrefix, results, false)
+	runRestartUnits(t, unitPrefix, templateResults, true)
+}
diff --git a/functional/scheduling_test.go b/functional/scheduling_test.go
index 6771df2e..2c0e1c8b 100644
--- a/functional/scheduling_test.go
+++ b/functional/scheduling_test.go
@@ -333,74 +333,6 @@ func TestScheduleOneWayConflict(t *testing.T) {
 
 }
 
-// Start 3 services that conflict with one another.
-func TestScheduleMultipleConflicts(t *testing.T) {
-	cluster, err := platform.NewNspawnCluster("smoke")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer cluster.Destroy(t)
-
-	// Start with a simple three-node cluster
-	members, err := platform.CreateNClusterMembers(cluster, 3)
-	if err != nil {
-		t.Fatal(err)
-	}
-	m0 := members[0]
-	machines, err := cluster.WaitForNMachines(m0, 3)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// Ensure we can SSH into each machine using fleetctl
-	for _, machine := range machines {
-		if stdout, stderr, err := cluster.Fleetctl(m0, "--strict-host-key-checking=false", "ssh", machine, "uptime"); err != nil {
-			t.Errorf("Unable to SSH into fleet machine: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
-		}
-	}
-
-	for i := 0; i < 3; i++ {
-		unit := fmt.Sprintf("fixtures/units/conflict.multiple.%d.service", i)
-		stdout, stderr, err := cluster.Fleetctl(m0, "start", "--no-block", unit)
-		if err != nil {
-			t.Errorf("Failed starting unit %s: \nstdout: %s\nstderr: %s\nerr: %v", unit, stdout, stderr, err)
-		}
-	}
-
-	// All 3 services should be visible immediately and 3 should become
-	// ACTIVE shortly thereafter
-	stdout, stderr, err := cluster.Fleetctl(m0, "list-unit-files", "--no-legend")
-	if err != nil {
-		t.Fatalf("Failed to run list-unit-files:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
-	}
-	units := strings.Split(strings.TrimSpace(stdout), "\n")
-	if len(units) != 3 {
-		t.Fatalf("Did not find five units in cluster: \n%s", stdout)
-	}
-	active, err := cluster.WaitForNActiveUnits(m0, 3)
-	if err != nil {
-		t.Fatal(err)
-	}
-	states, err := util.ActiveToSingleStates(active)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	machineSet := make(map[string]bool)
-
-	for unit, unitState := range states {
-		if len(unitState.Machine) == 0 {
-			t.Errorf("Unit %s is not reporting machine", unit)
-		}
-
-		machineSet[unitState.Machine] = true
-	}
-
-	if len(machineSet) != 3 {
-		t.Errorf("3 active units not running on 3 unique machines")
-	}
-}
-
 // TestScheduleReplace starts 3 units, followed by starting another unit
 // that replaces the 1st unit. Then it verifies that the original unit
 // got rescheduled on a different machine.
@@ -685,12 +617,14 @@ func TestScheduleGlobalUnits(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer cluster.Destroy(t)
-	members, err := platform.CreateNClusterMembers(cluster, 3)
+	numGUnits := 3
+	numAllUnits := 5
+	members, err := platform.CreateNClusterMembers(cluster, numGUnits)
 	if err != nil {
 		t.Fatal(err)
 	}
 	m0 := members[0]
-	machines, err := cluster.WaitForNMachines(m0, 3)
+	machines, err := cluster.WaitForNMachines(m0, numGUnits)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -708,19 +642,21 @@ func TestScheduleGlobalUnits(t *testing.T) {
 	}
 
 	// Now add a global unit
-	stdout, stderr, err = cluster.Fleetctl(m0, "start", "--no-block", "fixtures/units/global.service")
+	globalLongPath := "fixtures/units/global.service"
+	stdout, stderr, err = cluster.Fleetctl(m0, "start", "--no-block", globalLongPath)
 	if err != nil {
 		t.Fatalf("Failed starting unit: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
 	}
 
 	// Should see 2 + 3 units
-	states, err := cluster.WaitForNActiveUnits(m0, 5)
+	states, err := cluster.WaitForNActiveUnits(m0, numAllUnits)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	// Each machine should have a single global unit
-	us := states["global.service"]
+	globalBase := path.Base(globalLongPath)
+	us := states[globalBase]
 	for _, mach := range machines {
 		var found bool
 		for _, state := range us {
@@ -737,6 +673,27 @@ func TestScheduleGlobalUnits(t *testing.T) {
 			}
 		}
 	}
+
+	stdout, stderr, err = cluster.Fleetctl(m0, "status", "--no-block", globalBase)
+	if err != nil {
+		t.Fatalf("Failed getting unit status: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
+	}
+
+	// restarting global units
+	cmd := "restart"
+	stdout, stderr, err = cluster.Fleetctl(m0, cmd, globalBase)
+	if err != nil {
+		t.Fatalf("Failed restarting global unit: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
+	}
+
+	outmap, err := waitForNUnitsStart(cluster, m0, numAllUnits)
+	if err != nil {
+		t.Fatalf("Failed listing global units: %v", err)
+	}
+	glist, _ := outmap[globalBase]
+	if len(glist) != numGUnits {
+		t.Fatalf("Did not find %d global units: got %d", numGUnits, len(glist))
+	}
 }
 
 // TestScheduleGlobalConflicts starts 2 global units that conflict with each
diff --git a/functional/unit_action_test.go b/functional/unit_action_test.go
index 613161d4..602e5cc4 100644
--- a/functional/unit_action_test.go
+++ b/functional/unit_action_test.go
@@ -35,9 +35,10 @@ const (
 )
 
 var cleanCmd = map[string]string{
-	"submit": "destroy",
-	"load":   "unload",
-	"start":  "stop",
+	"submit":  "destroy",
+	"load":    "unload",
+	"start":   "stop",
+	"restart": "stop",
 }
 
 // TestUnitRunnable is the simplest test possible, deplying a single-node
@@ -165,6 +166,41 @@ func TestUnitStartReplace(t *testing.T) {
 	}
 }
 
+// TestUnitRestart checks if a unit becomes started and restarted successfully.
+// First it starts a unit, and restarts the unit, verifies it's restarted.
+func TestUnitRestart(t *testing.T) {
+	cluster, err := platform.NewNspawnCluster("smoke")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer cluster.Destroy(t)
+
+	m, err := cluster.CreateMember()
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = cluster.WaitForNMachines(m, 1)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	numUnits := 3
+
+	// first start units before restarting them
+	unitFiles, err := launchUnitsCmd(cluster, m, "start", numUnits)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := checkListUnits(cluster, m, "start", unitFiles, numUnits); err != nil {
+		t.Fatal(err)
+	}
+
+	// now restart
+	if err := unitStartCommon(cluster, m, "restart", numUnits); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestUnitSSHActions(t *testing.T) {
 	cluster, err := platform.NewNspawnCluster("smoke")
 	if err != nil {
@@ -582,6 +618,8 @@ func checkListUnits(cl platform.Cluster, m platform.Member, cmd string, ufs []st
 		lenLists = len(lus)
 		break
 	case "start":
+		fallthrough
+	case "restart":
 		lus, err = waitForNUnitsStart(cl, m, nu)
 		lenLists = len(lus)
 		break
@@ -604,18 +642,6 @@ func checkListUnits(cl platform.Cluster, m platform.Member, cmd string, ufs []st
 		if lenLists != nu || !found {
 			return fmt.Errorf("Expected %s to be unit file", ufs[i])
 		}
-
-		if cmd == "start" {
-			// Check expected systemd state after starting units
-			stdout, stderr, err := cl.MemberCommand(m, "systemctl", "show", "--property=ActiveState", ufs[i])
-			if strings.TrimSpace(stdout) != "ActiveState=active" {
-				return fmt.Errorf("Fleet unit not reported as active:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
-			}
-			stdout, stderr, err = cl.MemberCommand(m, "systemctl", "show", "--property=Result", ufs[i])
-			if strings.TrimSpace(stdout) != "Result=success" {
-				return fmt.Errorf("Result for fleet unit not reported as success:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err)
-			}
-		}
 	}
 	return err
 }
@@ -654,6 +680,8 @@ func waitForNUnitsCmd(cl platform.Cluster, m platform.Member, cmd string, nu int
 		_, err = waitForNUnitsLoad(cl, m, nu)
 		break
 	case "start":
+		fallthrough
+	case "restart":
 		_, err = waitForNUnitsStart(cl, m, nu)
 		break
 	default:
diff --git a/job/job_test.go b/job/job_test.go
index ba54b282..94c4be16 100644
--- a/job/job_test.go
+++ b/job/job_test.go
@@ -77,21 +77,6 @@ Description=Testing
 [X-Fleet]
 Conflicts=*bar*
 `, []string{"*bar*"}},
-		{``, []string{}},
-		{`[Unit]
-Description=Testing
-
-[X-Fleet]
-Conflicts=*bar* *foo*
-`, []string{"*bar*", "*foo*"}},
-		{``, []string{}},
-		{`[Unit]
-Description=Testing
-
-[X-Fleet]
-Conflicts=*bar*
-Conflicts=*foo*
-`, []string{"*bar*", "*foo*"}},
 	}
 	for i, tt := range testCases {
 		j := NewJob("echo.service", *newUnit(t, tt.contents))