Enforce idempotency of dispatched jobs using token on dispatch request

[alexmunda]

This PR adds the ability for users to supply an idempotency token when dispatching a parameterized job. When a token is supplied, Nomad will ensure that no existing child jobs contain a matching token. If a child job does have a matching idempotency token, the dispatch will fail.

I believe this will require documentation updates to the job Dispatch endpoint.

Addresses #10641.

[tgross]

Adding a new method here is good for backwards compatibility, but I'm wondering if in the long term it would be better to add a token to the WriteOptions struct instead. That would make it possible for us to add idempotency to any kind of write API in the future without adding a bunch of new methods.

Would love to hear thoughts from @DerekStrickland @schmichael @notnoop on that

[DerekStrickland]

I like the idea of putting the token on the WriteOptions. If feels like it enables an almost aspect oriented approach for handling this generically at more of a framework level.

[alexmunda]

Added to WriteOptions!

[DerekStrickland]

Thanks @alexmunda !

[notnoop]

I'm conflicted about having the field in WriteOptions and slightly lean towards how the PR had it before under JobDispatchRequest. Idempotency is such a common concern, so belonging to WriteOptions make sense. Though, it currently only applies to dispatch jobs; other APIs address a similar but not an exact problem using CAS and ModifyIndex (e.g. Update Scheduler Configuration endpoint).

I'm fine with having it in WriteOptions, as it's probably the pattern we want to use moving forward - but would recommend calling out that only the dispatch endpoint supports it.

[alexmunda]

@notnoop I am open to either approach 😄. With the current WriteOptions approach, where would the best place be to call that out? Comment on the struct field?

[tgross]

The comments on this section are kind of the i++ // increment i by 1 sort. I'd consider adding a comment to the top of this block that says describes the goal instead:

// Ensure that we have only one dispatched version of this job running concurrently
// by comparing the idempotency token against any non-terminal versions.

[alexmunda]

@notnoop Should reply.Index be ModifyIndex or CreateIndex? I went with ModifyIndex so the caller can diff the create and modify indexes and tell that an existing job was returned.

[notnoop]

I think ModifyIndex is good as well. Thanks!

[notnoop]

This looks great. Just realized that args.IdempotencyToken needs to be parsed and set in the http handler as well:

nomad/command/agent/job_endpoint.go

Lines 636 to 659 in 60a92ff

	func (s *HTTPServer) jobDispatchRequest(resp http.ResponseWriter, req *http.Request, name string) (interface{}, error) {
	if req.Method != "PUT" && req.Method != "POST" {
	return nil, CodedError(405, ErrInvalidMethod)
	}
	args := structs.JobDispatchRequest{}
	if err := decodeBody(req, &args); err != nil {
	return nil, CodedError(400, err.Error())
	}
	if args.JobID != "" && args.JobID != name {
	return nil, CodedError(400, "Job ID does not match")
	}
	if args.JobID == "" {
	args.JobID = name
	}
	s.parseWriteRequest(req, &args.WriteRequest)
	var out structs.JobDispatchResponse
	if err := s.agent.RPC("Job.Dispatch", &args, &out); err != nil {
	return nil, err
	}
	setIndex(resp, out.Index)
	return out, nil
	}

.

It will be nice to update the CLI as well to accept -idempotency-token argument but we, the nomad team, can follow up with that.

[notnoop]

LGTM. Thank you so much again!

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.