state store: fix logic for evaluating job status

nomad/fsm.go

@@ -948,20 +948,21 @@ func (n *nomadFSM) applyAllocClientUpdate(msgType structs.MessageType, buf []byt
 		}
 	}
 
-	// Update all the client allocations
-	if err := n.state.UpdateAllocsFromClient(msgType, index, req.Alloc); err != nil {
-		n.logger.Error("UpdateAllocFromClient failed", "error", err)
-		return err
-	}
-
-	// Update any evals
+	// Update any evals first.  During a reschedule, we don't want to mark the job dead, which
+	// would happen if all the allocs were terminal and there wasn't a pending eval
 	if len(req.Evals) > 0 {
 		if err := n.upsertEvals(msgType, index, req.Evals); err != nil {
 			n.logger.Error("applyAllocClientUpdate failed to update evaluations", "error", err)
 			return err
 		}
 	}
 
+	// Update all the client allocations
+	if err := n.state.UpdateAllocsFromClient(msgType, index, req.Alloc); err != nil {
+		n.logger.Error("UpdateAllocFromClient failed", "error", err)
+		return err
+	}
+
 	// Unblock evals for the nodes computed node class if the client has
 	// finished running an allocation.
 	for _, alloc := range req.Alloc {

nomad/job_endpoint.go

@@ -659,6 +659,10 @@ func (j *Job) Revert(args *structs.JobRevertRequest, reply *structs.JobRegisterR
 	// Clear out the VersionTag to prevent tag duplication
 	revJob.VersionTag = nil
 
+	// Set the stable flag to false as this is functionally a new registration
+	// and should handle deployment updates
+	revJob.Stable = false
+
 	reg := &structs.JobRegisterRequest{
 		Job:          revJob,
 		WriteRequest: args.WriteRequest,

nomad/state/state_store.go

@@ -3716,6 +3716,7 @@ func (s *StateStore) DeleteEval(index uint64, evals, allocs []string, userInitia
 		}
 	}
 
+	// TODO: should we really be doing this here?  We don't do it for filtered evals.
 	// Set the job's status
 	if err := s.setJobStatuses(index, txn, jobs, true); err != nil {
 		return fmt.Errorf("setting job status failed: %v", err)
@@ -3851,11 +3852,6 @@ func (s *StateStore) EvalsByJob(ws memdb.WatchSet, namespace, jobID string) ([]*
 
 		e := raw.(*structs.Evaluation)
 
-		// Filter non-exact matches
-		if e.JobID != jobID {
-			continue
-		}
-
 		out = append(out, e)
 	}
 	return out, nil
@@ -4824,14 +4820,13 @@ func (s *StateStore) UpdateDeploymentStatus(msgType structs.MessageType, index u
 		return err
 	}
 
-	// Upsert the job if necessary
+	// On failed deployments with auto_revert set to true, a new eval and job will be included on the request.
+	// We should upsert them both
 	if req.Job != nil {
 		if err := s.upsertJobImpl(index, nil, req.Job, false, txn); err != nil {
 			return err
 		}
 	}
-
-	// Upsert the optional eval
 	if req.Eval != nil {
 		if err := s.nestedUpsertEval(txn, index, req.Eval); err != nil {
 			return err
@@ -5532,6 +5527,12 @@ func (s *StateStore) setJobStatus(index uint64, txn *txn,
 	if err := s.setJobSummary(txn, updated, index, oldStatus, newStatus); err != nil {
 		return fmt.Errorf("job summary update failed %w", err)
 	}
+
+	// Update the job version details
+	if err := s.upsertJobVersion(index, updated, txn); err != nil {
+		return err
+	}
+
 	return nil
 }
 
@@ -5599,14 +5600,16 @@ func (s *StateStore) setJobSummary(txn *txn, updated *structs.Job, index uint64,
 }
 
 func (s *StateStore) getJobStatus(txn *txn, job *structs.Job, evalDelete bool) (string, error) {
+	// If the job has been stopped, it's status is dead
+	if job.Stopped() {
+		return structs.JobStatusDead, nil
+	}
+
 	// System, Periodic and Parameterized jobs are running until explicitly
 	// stopped.
 	if job.Type == structs.JobTypeSystem ||
 		job.IsParameterized() ||
 		job.IsPeriodic() {
-		if job.Stop {
-			return structs.JobStatusDead, nil
-		}
 		return structs.JobStatusRunning, nil
 	}
 
@@ -5616,40 +5619,42 @@ func (s *StateStore) getJobStatus(txn *txn, job *structs.Job, evalDelete bool) (
 	}
 
 	// If there is a non-terminal allocation, the job is running.
-	hasAlloc := false
+	terminalAllocs := false
 	for alloc := allocs.Next(); alloc != nil; alloc = allocs.Next() {
-		hasAlloc = true
-		if !alloc.(*structs.Allocation).TerminalStatus() {
+		a := alloc.(*structs.Allocation)
+
+		if a.Job.Version < job.Version {
+			continue
+		}
+
+		if !a.TerminalStatus() {
 			return structs.JobStatusRunning, nil
 		}
+
+		terminalAllocs = true
 	}
 
 	evals, err := txn.Get("evals", "job_prefix", job.Namespace, job.ID)
 	if err != nil {
 		return "", err
 	}
 
-	hasEval := false
 	for raw := evals.Next(); raw != nil; raw = evals.Next() {
 		e := raw.(*structs.Evaluation)
 
-		// Filter non-exact matches
-		if e.JobID != job.ID {
-			continue
-		}
-
-		hasEval = true
 		if !e.TerminalStatus() {
 			return structs.JobStatusPending, nil
 		}
 	}
 
-	// The job is dead if all the allocations and evals are terminal or if there
-	// are no evals because of garbage collection.
-	if evalDelete || hasEval || hasAlloc {
+	// The job is dead if all allocations for this version are terminal.
+	if terminalAllocs {
 		return structs.JobStatusDead, nil
 	}
 
+	// There are no allocs yet, which will happen for new job submissions,
+	// running new versions of a job, or reverting. This will happen if
+	// the evaluation is persisted after the job is persisted
 	return structs.JobStatusPending, nil
 }