agent: immediately load (and possibly launch) differing units

coreos · Oct 24, 2014 · bfd31a2 · bfd31a2
1 parent a978c3b
commit bfd31a2
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 22 deletions.
diff --git a/agent/agent.go b/agent/agent.go
@@ -137,23 +137,23 @@ func (a *Agent) units() (unitStates, error) {
 		loaded.Add(jName)
 	}
 
-	unitFiles, err := a.um.Units()
+	units, err := a.um.Units()
 	if err != nil {
 		return nil, fmt.Errorf("failed fetching loaded units from UnitManager: %v", err)
 	}
 
 	filter := pkg.NewUnsafeSet()
-	for _, u := range unitFiles {
+	for _, u := range units {
 		filter.Add(u)
 	}
 
-	units, err := a.um.GetUnitStates(filter)
+	uStates, err := a.um.GetUnitStates(filter)
 	if err != nil {
 		return nil, fmt.Errorf("failed fetching unit states from UnitManager: %v", err)
 	}
 
 	states := make(unitStates)
-	for uName, state := range units {
+	for uName, uState := range uStates {
 		js := job.JobStateInactive
 		if loaded.Contains(uName) {
 			js = job.JobStateLoaded
@@ -162,7 +162,7 @@ func (a *Agent) units() (unitStates, error) {
 		}
 		us := unitState{
 			state: js,
-			hash:  state.UnitHash,
+			hash:  uState.UnitHash,
 		}
 		states[uName] = us
 	}

diff --git a/agent/reconcile.go b/agent/reconcile.go
@@ -236,7 +236,7 @@ func (ar *AgentReconciler) calculateTaskChainForUnit(dState *AgentState, cState
 			reason: taskReasonScheduledButUnloaded,
 		})
 
-		// as an optimization, queue the job for launching immediately after loading
+		// as an optimization, queue the unit for launching immediately after loading
 		if dJob.TargetState == job.JobStateLaunched {
 			tc.Add(task{
 				typ:    taskTypeStartUnit,
@@ -249,14 +249,26 @@ func (ar *AgentReconciler) calculateTaskChainForUnit(dState *AgentState, cState
 
 	if cJHash != dJHash {
 		log.V(1).Infof("Desired hash %q differs to current hash %s of Job(%s) - unloading", dJHash, cJHash, jName)
-		t := task{
+		tc := newTaskChain(u)
+		tc.Add(task{
 			typ:    taskTypeUnloadUnit,
 			reason: taskReasonLoadedButHashDiffers,
+		})
+
+		// queue the correct unit for loading immediately after unloading the old one
+		tc.Add(task{
+			typ:    taskTypeLoadUnit,
+			reason: taskReasonScheduledButUnloaded,
+		})
+
+		// as an optimization, queue the unit for launching immediately after loading
+		if dJob.TargetState == job.JobStateLaunched {
+			tc.Add(task{
+				typ:    taskTypeStartUnit,
+				reason: taskReasonLoadedDesiredStateLaunched,
+			})
 		}
-		u := &job.Unit{
-			Name: jName,
-		}
-		tc := newTaskChain(u, t)
+
 		return &tc
 	}
 

diff --git a/agent/reconcile_test.go b/agent/reconcile_test.go
@@ -26,6 +26,10 @@ import (
 	"github.com/coreos/fleet/unit"
 )
 
+// Represents the hash of e.g. an empty unit
+// echo -n "" | sha1sum
+const emptyStringHash = "da39a3ee5e6b4b0d3255bfef95601890afd80709"
+
 var (
 	jsInactive = job.JobStateInactive
 	jsLoaded   = job.JobStateLoaded
@@ -430,8 +434,7 @@ func TestCalculateTasksForJob(t *testing.T) {
 			cState: unitStates{
 				"foo.service": unitState{
 					state: jsLoaded,
-					// hash of empty unit
-					hash: "da39a3ee5e6b4b0d3255bfef95601890afd80709",
+					hash:  emptyStringHash,
 				},
 			},
 			uName: "foo.service",
@@ -449,15 +452,14 @@ func TestCalculateTasksForJob(t *testing.T) {
 			cState: unitStates{
 				"foo.service": unitState{
 					state: jsLaunched,
-					// hash of empty unit
-					hash: "da39a3ee5e6b4b0d3255bfef95601890afd80709",
+					hash:  emptyStringHash,
 				},
 			},
 			uName: "foo.service",
 			chain: nil,
 		},
 
-		// when current state == desired state but hash differs, unit should be unloaded
+		// when current state == desired state but hash differs, unit should be unloaded and then reloaded
 		{
 			dState: &AgentState{
 				MState: &machine.MachineState{ID: "XXX"},
@@ -482,21 +484,29 @@ func TestCalculateTasksForJob(t *testing.T) {
 						typ:    taskTypeUnloadUnit,
 						reason: taskReasonLoadedButHashDiffers,
 					},
+					task{
+						typ:    taskTypeLoadUnit,
+						reason: taskReasonScheduledButUnloaded,
+					},
+					task{
+						typ:    taskTypeStartUnit,
+						reason: taskReasonLoadedDesiredStateLaunched,
+					},
 				},
 			},
 		},
 
-		// when current state != desired state and hash differs, unit should be unloaded
+		// when current state != desired state and hash differs, unit should be unloaded and then reloaded
 		{
 			dState: &AgentState{
 				MState: &machine.MachineState{ID: "XXX"},
 				Units: map[string]*job.Unit{
-					"foo.service": &job.Unit{TargetState: jsLaunched},
+					"foo.service": &job.Unit{TargetState: jsLoaded},
 				},
 			},
 			cState: unitStates{
 				"foo.service": unitState{
-					state: jsLoaded,
+					state: jsLaunched,
 					hash:  "abcdefg",
 				},
 			},
@@ -511,21 +521,25 @@ func TestCalculateTasksForJob(t *testing.T) {
 						typ:    taskTypeUnloadUnit,
 						reason: taskReasonLoadedButHashDiffers,
 					},
+					task{
+						typ:    taskTypeLoadUnit,
+						reason: taskReasonScheduledButUnloaded,
+					},
 				},
 			},
 		},
 
-		// when current state != desired state and hash differs, unit should be unloaded
+		// when current state != desired state and hash differs, unit should be unloaded and then reloaded
 		{
 			dState: &AgentState{
 				MState: &machine.MachineState{ID: "XXX"},
 				Units: map[string]*job.Unit{
-					"foo.service": &job.Unit{TargetState: jsLoaded},
+					"foo.service": &job.Unit{TargetState: jsLaunched},
 				},
 			},
 			cState: unitStates{
 				"foo.service": unitState{
-					state: jsLaunched,
+					state: jsLoaded,
 					hash:  "abcdefg",
 				},
 			},
@@ -540,6 +554,14 @@ func TestCalculateTasksForJob(t *testing.T) {
 						typ:    taskTypeUnloadUnit,
 						reason: taskReasonLoadedButHashDiffers,
 					},
+					task{
+						typ:    taskTypeLoadUnit,
+						reason: taskReasonScheduledButUnloaded,
+					},
+					task{
+						typ:    taskTypeStartUnit,
+						reason: taskReasonLoadedDesiredStateLaunched,
+					},
 				},
 			},
 		},
@@ -614,6 +636,7 @@ func TestCalculateTasksForJob(t *testing.T) {
 			cState: unitStates{
 				"foo.service": unitState{
 					state: jsLoaded,
+					hash:  emptyStringHash,
 				},
 			},
 			uName: "foo.service",
@@ -636,6 +659,7 @@ func TestCalculateTasksForJob(t *testing.T) {
 			cState: unitStates{
 				"foo.service": unitState{
 					state: jsLaunched,
+					hash:  emptyStringHash,
 				},
 			},
 			uName: "foo.service",
@@ -665,6 +689,7 @@ func TestCalculateTasksForJob(t *testing.T) {
 			cState: unitStates{
 				"foo.service": unitState{
 					state: jsLoaded,
+					hash:  emptyStringHash,
 				},
 			},
 			uName: "foo.service",
@@ -687,6 +712,12 @@ func TestCalculateTasksForJob(t *testing.T) {
 		chain := ar.calculateTaskChainForUnit(tt.dState, tt.cState, tt.uName)
 		if !reflect.DeepEqual(tt.chain, chain) {
 			t.Errorf("case %d: calculated incorrect task chain\nexpected=%#v\nreceived=%#v\n", i, tt.chain, chain)
+			if c := tt.chain; c != nil {
+				t.Logf("expected Unit: %#v", *c.unit)
+			}
+			if c := chain; c != nil {
+				t.Logf("received Unit: %#v", *c.unit)
+			}
 		}
 	}
 }