Merge pull request #1639 from endocode/dongsu/fleetd-recheck-nil-hash

agent: reenable check for nil hash in unit state publisher
coreos · Jul 22, 2016 · f8b2ad2 · f8b2ad2
2 parents 0780574 + d77b8e4
commit f8b2ad2
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 27 deletions.
diff --git a/agent/unit_state.go b/agent/unit_state.go
@@ -207,12 +207,9 @@ func newPublisher(reg registry.Registry, ttl time.Duration) publishFunc {
 			// Sanity check - don't want to publish incomplete UnitStates
 			// TODO(jonboulle): consider teasing apart a separate UnitState-like struct
 			// so we can rely on a UnitState always being fully hydrated?
-
-			// See https://github.com/coreos/fleet/issues/720
-			//if len(us.UnitHash) == 0 {
-			//	log.Errorf("Refusing to push UnitState(%s), no UnitHash: %#v", name, us)
-
-			if len(us.MachineID) == 0 {
+			if len(us.UnitHash) == 0 {
+				log.Errorf("Refusing to push UnitState(%s), no UnitHash: %#v", name, us)
+			} else if len(us.MachineID) == 0 {
 				log.Errorf("Refusing to push UnitState(%s), no MachineID: %#v", name, us)
 			} else {
 				log.Debugf("Pushing UnitState(%s) to Registry: %#v", name, us)

diff --git a/agent/unit_state_test.go b/agent/unit_state_test.go
@@ -227,13 +227,15 @@ func TestDefaultPublisher(t *testing.T) {
 				UnitName:    "foo.service",
 				ActiveState: "active",
 				MachineID:   "xyz",
+				UnitHash:    "quickbrownfox",
 			},
 			nil,
 			[]*unit.UnitState{
 				&unit.UnitState{
 					UnitName:    "foo.service",
 					ActiveState: "active",
 					MachineID:   "xyz",
+					UnitHash:    "quickbrownfox",
 				},
 			},
 		},
@@ -243,6 +245,7 @@ func TestDefaultPublisher(t *testing.T) {
 			&unit.UnitState{
 				UnitName:    "foo.service",
 				ActiveState: "active",
+				UnitHash:    "quickbrownfox",
 			},
 			nil,
 			[]*unit.UnitState{},
@@ -255,6 +258,7 @@ func TestDefaultPublisher(t *testing.T) {
 				unit.UnitState{
 					UnitName:    "foo.service",
 					ActiveState: "active",
+					UnitHash:    "quickbrownfox",
 				},
 			},
 			[]*unit.UnitState{},

diff --git a/registry/unit_state.go b/registry/unit_state.go
@@ -224,10 +224,9 @@ func unitStateToModel(us *unit.UnitState) *unitStateModel {
 	}
 
 	// Refuse to create a UnitState without a Hash
-	// See https://github.com/coreos/fleet/issues/720
-	//if len(us.UnitHash) == 0 {
-	//	return nil
-	//}
+	if len(us.UnitHash) == 0 {
+		return nil
+	}
 
 	usm := unitStateModel{
 		LoadState:   us.LoadState,

diff --git a/registry/unit_state_test.go b/registry/unit_state_test.go
@@ -111,14 +111,13 @@ func TestSaveUnitState(t *testing.T) {
 		t.Fatalf("SaveUnitState of nil state should fail but acted unexpectedly!")
 	}
 
-	// Saving unit state with no hash should succeed for now, but should fail
-	// in the future. See https://github.com/coreos/fleet/issues/720.
-	//r.SaveUnitState(j, us, time.Second)
-	//if len(e.sets) != 1 || e.deletes == nil {
-	//	t.Logf("sets: %#v", e.sets)
-	//	t.Logf("deletes: %#v", e.deletes)
-	//	t.Fatalf("SaveUnitState on UnitState with no hash acted unexpectedly!")
-	//}
+	// Saving unit state with no hash should should fail.
+	r.SaveUnitState(j, us, time.Second)
+	if len(e.sets) != 0 || e.deletes != nil {
+		t.Logf("sets: %#v", e.sets)
+		t.Logf("deletes: %#v", e.deletes)
+		t.Fatalf("SaveUnitState on UnitState with no hash acted unexpectedly!")
+	}
 
 	us.UnitHash = "quickbrownfox"
 	r.SaveUnitState(j, us, time.Second)
@@ -195,8 +194,7 @@ func TestUnitStateToModel(t *testing.T) {
 			want: nil,
 		},
 		{
-			// Unit state with no hash and no machineID is OK
-			// See https://github.com/coreos/fleet/issues/720
+			// Unit state with no hash and no machineID is not OK
 			in: &unit.UnitState{
 				LoadState:   "foo",
 				ActiveState: "bar",
@@ -205,13 +203,7 @@ func TestUnitStateToModel(t *testing.T) {
 				UnitHash:    "",
 				UnitName:    "name",
 			},
-			want: &unitStateModel{
-				LoadState:    "foo",
-				ActiveState:  "bar",
-				SubState:     "baz",
-				MachineState: nil,
-				UnitHash:     "",
-			},
+			want: nil,
 		},
 		{
 			// Unit state with hash but no machineID is OK