Repository: hashicorp/nomad
File: nomad/job_endpoint.go (Go)

Code before:
	// Submit a multiregion job to other regions (enterprise only).
	// The job will have its region interpolated.
	var newVersion uint64
	if existingJob != nil {
		newVersion = existingJob.Version + 1
	}
	isRunner, err := j.multiregionRegister(args, reply, newVersion)
	if err != nil {
		return err
	}

	// Set the submit time
	now := time.Now().UnixNano()
	args.Job.SubmitTime = now

	// If the job is periodic or parameterized, we don't create an eval.
	if !(args.Job.IsPeriodic() || args.Job.IsParameterized()) {

		// Initially set the eval priority to that of the job priority. If the
		// user supplied an eval priority override, we subsequently use this.
		evalPriority := args.Job.Priority
		if args.EvalPriority > 0 {
			evalPriority = args.EvalPriority
		}

		args.Eval = &structs.Evaluation{
			ID:          uuid.Generate(),
			Namespace:   args.RequestNamespace(),
			Priority:    evalPriority,
			Type:        args.Job.Type,
			TriggeredBy: structs.EvalTriggerJobRegister,
			JobID:       args.Job.ID,
			Status:      structs.EvalStatusPending,
			CreateTime:  now,
			ModifyTime:  now,
		}
		reply.EvalID = args.Eval.ID
	}

	// Check if the job has changed at all
	specChanged, err := j.multiregionSpecChanged(existingJob, args)
	if err != nil {
		return err
	}
	// We know the job isn't nil, so if the spec hasn't changed,
	// there is an existing job.
	if !specChanged {
		reply.JobModifyIndex = existingJob.ModifyIndex

		// Force an eval for service/batch jobs
		if args.Eval != nil {

Review comment (suggestion):
I _think_ having this field on the request is a weird side-effect of our reuse of the same structs for on-the-wire and storage. So it _should_ always be nil until set here. But previously we'd only overwrite any eval from the request if the spec changed.

I've checked through the code and I don't see anywhere (in production code) where we're setting `args.Eval` before it gets to the handler, and in the worst case scenario we'd just create a new eval and it doesn't matter anyways. So I'm pretty sure this is safe... but just wanted to point this out for you and any other reviewers and to see if it tickles any thoughts about what I might have missed.

Code after:
	// Submit a multiregion job to other regions (enterprise only).
	// The job will have its region interpolated.
	var newVersion uint64
	if existingJob != nil {
		newVersion = existingJob.Version + 1
	}
	isRunner, err := j.multiregionRegister(args, reply, newVersion)
	if err != nil {
		return err
	}

	// Set the submit time
	now := time.Now().UnixNano()
	args.Job.SubmitTime = now

	// If the job is periodic or parameterized, we don't create an eval.
	if !(args.Job.IsPeriodic() || args.Job.IsParameterized()) {

		// Initially set the eval priority to that of the job priority. If the
		// user supplied an eval priority override, we subsequently use this.
		evalPriority := args.Job.Priority
		if args.EvalPriority > 0 {
			evalPriority = args.EvalPriority
		}

		args.Eval = &structs.Evaluation{
			ID:          uuid.Generate(),
			Namespace:   args.RequestNamespace(),
			Priority:    evalPriority,
			Type:        args.Job.Type,
			TriggeredBy: structs.EvalTriggerJobRegister,
			JobID:       args.Job.ID,
			Status:      structs.EvalStatusPending,
			CreateTime:  now,
			ModifyTime:  now,
		}
		reply.EvalID = args.Eval.ID
	}

	// Check if the job has changed at all
	specChanged, err := j.multiregionSpecChanged(existingJob, args)
	if err != nil {
		return err
	}
	// We know the job isn't nil, so if the spec hasn't changed,
	// there is an existing job.
	if !specChanged {
		reply.JobModifyIndex = existingJob.ModifyIndex

		// Force an eval for service/batch jobs. This is intentional
		// behavior to allow triggering of evaluations via re-registering