From 85129bb7943142d991122afcb890ab0baa598603 Mon Sep 17 00:00:00 2001 From: Drew Bailey Date: Mon, 25 Jan 2021 10:34:27 -0500 Subject: [PATCH] ignore setting job summary when oldstatus == newstatus (#9884) --- nomad/state/state_store.go | 13 +++----- nomad/state/state_store_test.go | 57 +++++++++++++-------------------- 2 files changed, 27 insertions(+), 43 deletions(-) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index bcbc6c16787..19f088ed778 100644 --- 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 index 1c42e677f1c..8f01cac7835 100644 --- 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,16 +5022,13 @@ 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 @@ -5034,40 +5036,27 @@ func TestStateStore_UpsertAlloc_ChildJob(t *testing.T) { // 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) {