ignore setting job summary when oldstatus == newstatus (#9884)

hashicorp · Jan 25, 2021 · 85129bb · 85129bb
1 parent ae91c83
commit 85129bb
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 43 deletions.
diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go
@@ -4428,7 +4428,6 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn,
 
 	// Capture the current status so we can check if there is a change
 	oldStatus := job.Status
-	firstPass := index == job.CreateIndex
 	newStatus := forceStatus
 
 	// If forceStatus is not set, compute the jobs status.
@@ -4440,12 +4439,8 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn,
 		}
 	}
 
-	// Fast-path if the job has changed.
-	// Still update the job summary if necessary.
+	// Fast-path if the job has not changed.
 	if oldStatus == newStatus {
-		if err := s.setJobSummary(txn, job, index, oldStatus, newStatus, firstPass); err != nil {
-			return err
-		}
 		return nil
 	}
 
@@ -4463,13 +4458,13 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn,
 	}
 
 	// Update the children summary
-	if err := s.setJobSummary(txn, updated, index, oldStatus, newStatus, firstPass); err != nil {
+	if err := s.setJobSummary(txn, updated, index, oldStatus, newStatus); err != nil {
 		return fmt.Errorf("job summary update failed %w", err)
 	}
 	return nil
 }
 
-func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64, oldStatus, newStatus string, firstPass bool) error {
+func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64, oldStatus, newStatus string) error {
 	if updated.ParentID == "" {
 		return nil
 	}
@@ -4493,7 +4488,7 @@ func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64,
 		children := pSummary.Children
 
 		// Decrement old status
-		if !firstPass {
+		if oldStatus != "" {
 			switch oldStatus {
 			case structs.JobStatusPending:
 				children.Pending--

diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go
@@ -1659,6 +1659,7 @@ func TestStateStore_UpsertJob_ChildJob(t *testing.T) {
 	}
 
 	child := mock.Job()
+	child.Status = ""
 	child.ParentID = parent.ID
 	if err := state.UpsertJob(structs.MsgTypeTestSetup, 1001, child); err != nil {
 		t.Fatalf("err: %v", err)
@@ -1996,6 +1997,7 @@ func TestStateStore_DeleteJob_ChildJob(t *testing.T) {
 	}
 
 	child := mock.Job()
+	child.Status = ""
 	child.ParentID = parent.ID
 
 	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil {
@@ -4022,6 +4024,7 @@ func TestStateStore_UpsertEvals_Eval_ChildJob(t *testing.T) {
 	}
 
 	child := mock.Job()
+	child.Status = ""
 	child.ParentID = parent.ID
 
 	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil {
@@ -4246,6 +4249,7 @@ func TestStateStore_DeleteEval_ChildJob(t *testing.T) {
 	}
 
 	child := mock.Job()
+	child.Status = ""
 	child.ParentID = parent.ID
 
 	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil {
@@ -4498,6 +4502,7 @@ func TestStateStore_UpdateAllocsFromClient(t *testing.T) {
 	}
 
 	child := mock.Job()
+	child.Status = ""
 	child.ParentID = parent.ID
 	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil {
 		t.Fatalf("err: %v", err)
@@ -5017,57 +5022,41 @@ func TestStateStore_UpsertAlloc_ChildJob(t *testing.T) {
 	state := testStateStore(t)
 
 	parent := mock.Job()
-	if err := state.UpsertJob(structs.MsgTypeTestSetup, 998, parent); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 998, parent))
 
 	child := mock.Job()
+	child.Status = ""
 	child.ParentID = parent.ID
 
-	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, child); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 999, child))
 
 	alloc := mock.Alloc()
 	alloc.JobID = child.ID
 	alloc.Job = child
 
 	// Create watchsets so we can test that delete fires the watch
 	ws := memdb.NewWatchSet()
-	if _, err := state.JobSummaryByID(ws, parent.Namespace, parent.ID); err != nil {
-		t.Fatalf("bad: %v", err)
-	}
+	_, err := state.JobSummaryByID(ws, parent.Namespace, parent.ID)
+	require.NoError(t, err)
 
-	err := state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc})
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	err = state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc})
+	require.NoError(t, err)
 
-	if !watchFired(ws) {
-		t.Fatalf("bad")
-	}
+	require.True(t, watchFired(ws))
 
 	ws = memdb.NewWatchSet()
 	summary, err := state.JobSummaryByID(ws, parent.Namespace, parent.ID)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if summary == nil {
-		t.Fatalf("nil summary")
-	}
-	if summary.JobID != parent.ID {
-		t.Fatalf("bad summary id: %v", parent.ID)
-	}
-	if summary.Children == nil {
-		t.Fatalf("nil children summary")
-	}
-	if summary.Children.Pending != 0 || summary.Children.Running != 1 || summary.Children.Dead != 0 {
-		t.Fatalf("bad children summary: %v", summary.Children)
-	}
+	require.NoError(t, err)
+	require.NotNil(t, summary)
 
-	if watchFired(ws) {
-		t.Fatalf("bad")
-	}
+	require.Equal(t, parent.ID, summary.JobID)
+	require.NotNil(t, summary.Children)
+
+	require.Equal(t, int64(0), summary.Children.Pending)
+	require.Equal(t, int64(1), summary.Children.Running)
+	require.Equal(t, int64(0), summary.Children.Dead)
+
+	require.False(t, watchFired(ws))
 }
 
 func TestStateStore_UpdateAlloc_Alloc(t *testing.T) {