Add priority flag to Dispatch CLI and API | NET-10171

[tehut]

Description

This PR addresses #10803 by adding a -priority flag directly to the nomad job dispatch CLI command.

To do so we:
jobs.go

• Created a new DispatchOptions struct with the new priority field as well as all the current Dispatch options.
• Moved the logic of the Dispatch() endpoint to a new DispatchOpts() HTTP API endpoint that takes the DispatchOptions stuct and is used under the cover by Dispatch()

command/job_dispatch.go

• added the priority flag and replaced the call to Dispatch() with one to DispatchOpts()

nomad/job_endpoint.go
Updated the Dispatch() function to:

• Overwrite the value of args.Priority with the value in the parent job if it is set to 0/unset
• Always set Priority on the dispatchJob == args.Priority (which will either be the parent value or a new value > 0)
• Update the signature of validateDispatchRequest() (not used outside this file) to accept a *Config
• Added a stanza at the end of validateDispatchRequest() that replicates the min/max priority value check from jobValidator.Validate() and applies it to args.Priority.

nomad/structs/structs.go

• Added a Priority field to the JobDispatchRequest

website/..../dispatch.mdx

• Added - priority to the list of dispatch options

Testing & Reproduction steps

N/A

Links

GH Issue: #10803

Contributor Checklist

• Changelog Entry If this PR changes user-facing behavior, please generate and add a
  changelog entry using the make cl command.
• Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
  ensure regressions will be caught.
• Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
  and job configuration, please update the Nomad website documentation to reflect this. Refer to
  the website README for docs guidelines. Please also consider whether the
  change requires notes within the upgrade guide.

Reviewer Checklist

• Backport Labels Please add the correct backport labels as described by the internal
  backporting document.
• Commit Type Ensure the correct merge method is selected which should be "squash and merge"
  in the majority of situations. The main exceptions are long-lived feature branches or merges where
  history should be preserved.
• Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
  within the public repository.

[aimeeu]

Thanks for updating the docs.

[tgross]

This is looking really good @tehut! I've left a bunch of comments around the tests and the use of the shoenig/test/must API. But once those are resolved I think we'll be in good shape to ship this.

[tgross]

Jobs have a minimum priority of 1, so I don't think we want to floor the dispatch priority to 50. But it also doesn't look like there's a code change here to enforce this floor, so this might just be a docs issue?

[tgross]

If we have an unexpected error, must.NoError will report it, so we don't need this bit anymore.

Suggested change

	if err != nil {
	t.Logf("err: %v", err)
	}

[tgross]

We don't need this block anymore either (I can't leave a suggestion because there's a deleted line in here).

[tgross]

This no longer exits on error so we've broken the assertion here. Looks like this block could be turned into must.NoError(t, err) instead.

[tgross]

We have must.NotEq(t, "", dispatch.EvalID) below, which will log if we fail the assertion, so we can remove this whole block. Same applies to the assertion after that.

[tgross]

Nitpick: keeping test case text align makes it easier to scan

Suggested change

	{name: "invalid priority",
	{
	name: "invalid priority",

[tgross]

I realize this is existing code, but this is asserting the test case field. I'd probably refactor this test a little bit to combine tc.err and tc.errStr into a tc.expectError field (of type error), and then do something along the lines of:

if tc.expectError != nil {
	must.ErrorContains(t, dispatchErr, tc.expectError.Error())
} else {
	must.NoError(t, dispatchErr)
}

I think you'll find that pattern gets used in a lot of our tests.

[tgross]

The existing test code has a similar issue to what I've pointed out above, where we're making an assertion on the test case field.

[tgross]

We've already asserted this above, which will log. If they weren't equal, this block would be unreachable anyways.

Suggested change

	if out.CreateIndex != dispatchResp.JobCreateIndex {
	t.Logf("index mis-match")
	}

Similar for other logs in this test.

[tgross]

I'd make sure that tc.priority is always set (ex. it could be tc.expectPriority), so that we can see it at a glance in the test case list and then there's no conditional here -- we just always call must.Eq.

[tgross]

LGTM! Nice work!

Now that Nomad 1.10.0 has shipped, new features want to land in the 1.10.x branch (see this backport labels table). I've added the backport label for 1.10.x. Once you squash-merge this (give the squash commit a nice commit message too), a backport PR will be created for you to approve and squash-merge.

[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.