review: initial updates based on PR review.

api/api_test.go

@@ -108,7 +108,7 @@ func TestRequestTime(t *testing.T) {
 		t.Errorf("bad request time: %d", wm.RequestTime)
 	}
 
-	wm, err = client.delete("/", &out, nil, nil)
+	wm, err = client.delete("/", nil, &out, nil)
 	if err != nil {
 		t.Fatalf("delete err: %v", err)
 	}

api/evaluations.go

@@ -41,11 +41,11 @@ func (e *Evaluations) Info(evalID string, q *QueryOptions) (*Evaluation, *QueryM
 }
 
 // Delete is used to batch delete evaluations using their IDs.
-func (e *Evaluations) Delete(evalIDs []string, q *WriteOptions) (*WriteMeta, error) {
+func (e *Evaluations) Delete(evalIDs []string, w *WriteOptions) (*WriteMeta, error) {
 	req := EvalDeleteRequest{
 		EvalIDs: evalIDs,
 	}
-	wm, err := e.client.delete("/v1/evaluations", &req, nil, q)
+	wm, err := e.client.delete("/v1/evaluations", &req, nil, w)
 	if err != nil {
 		return nil, err
 	}

api/evaluations_test.go

@@ -157,7 +157,7 @@ func TestEvaluations_Delete(t *testing.T) {
 	// should return an error.
 	wm, err := testClient.Evaluations().Delete([]string{"8E231CF4-CA48-43FF-B694-5801E69E22FA"}, nil)
 	require.Nil(t, wm)
-	require.Contains(t, err.Error(), "eval broker is enabled")
+	require.ErrorContains(t, err, "eval broker is enabled")
 
 	// Pause the eval broker, and try to delete an evaluation that does not
 	// exist.
@@ -171,7 +171,7 @@ func TestEvaluations_Delete(t *testing.T) {
 	require.True(t, schedulerConfigUpdated.Updated)
 
 	wm, err = testClient.Evaluations().Delete([]string{"8E231CF4-CA48-43FF-B694-5801E69E22FA"}, nil)
-	require.Contains(t, err.Error(), "eval not found")
+	require.ErrorContains(t, err, "eval not found")
 }
 
 func TestEvaluations_Allocations(t *testing.T) {

command/agent/eval_endpoint.go

@@ -51,7 +51,7 @@ func (s *HTTPServer) evalsDeleteRequest(resp http.ResponseWriter, req *http.Requ
 	var args structs.EvalDeleteRequest
 
 	if err := decodeBody(req, &args); err != nil {
-		return nil, CodedError(http.StatusInternalServerError, err.Error())
+		return nil, CodedError(http.StatusBadRequest, err.Error())
 	}
 
 	numIDs := len(args.EvalIDs)
@@ -62,7 +62,7 @@ func (s *HTTPServer) evalsDeleteRequest(resp http.ResponseWriter, req *http.Requ
 		return nil, CodedError(http.StatusBadRequest, "request does not include any evaluation IDs")
 	} else if numIDs > structs.MaxUUIDsPerWriteRequest {
 		return nil, CodedError(http.StatusBadRequest, fmt.Sprintf(
-			"request includes %v evaluations IDs, should be %v or below",
+			"request includes %v evaluations IDs, must be %v or fewer",
 			numIDs, structs.MaxUUIDsPerWriteRequest))
 	}
 

command/agent/eval_endpoint_test.go

@@ -167,7 +167,7 @@ func TestHTTP_EvalsDelete(t *testing.T) {
 					obj, err := s.Server.EvalsRequest(respW, req)
 					require.Equal(t,
 						CodedError(http.StatusBadRequest,
-							"request includes 8000 evaluations IDs, should be 7281 or below"), err)
+							"request includes 8000 evaluations IDs, must be 7281 or fewer"), err)
 					require.Nil(t, obj)
 				})
 			},

command/eval_delete.go

@@ -177,7 +177,7 @@ func (e *EvalDeleteCommand) Run(args []string) int {
 	// influence this.
 	var exitCode int
 
-	// Call the correct function in order to handle to operator input
+	// Call the correct function in order to handle the operator input
 	// correctly.
 	switch len(args) {
 	case 1:
@@ -208,13 +208,11 @@ func (e *EvalDeleteCommand) Run(args []string) int {
 	// Always attempt to revert the state of the eval broker. In the event this
 	// succeeds, but the eval deletion failed, maintain the non-zero exit code
 	// so failures are clear.
-	return func(exitCode int) int {
-		if err := e.revertEvalBroker(); err != nil {
-			e.Ui.Error(fmt.Sprintf("Error reverting eval broker state: %s", err))
-			exitCode = 1
-		}
-		return exitCode
-	}(exitCode)
+	if err := e.revertEvalBroker(); err != nil {
+		e.Ui.Error(fmt.Sprintf("Error reverting eval broker state: %s", err))
+		exitCode = 1
+	}
+	return exitCode
 }
 
 // verifyArgsAndFlags ensures the passed arguments and flags are valid for what
@@ -288,7 +286,7 @@ func (e *EvalDeleteCommand) revertEvalBroker() error {
 
 	// If the broker is paused, and it was paused before this command was
 	// invoked, there is nothing else to do.
-	if e.originalBrokerPaused == schedulerConfig.SchedulerConfig.PauseEvalBroker {
+	if schedulerConfig.SchedulerConfig.PauseEvalBroker && e.originalBrokerPaused {
 		return nil
 	}
 
@@ -319,26 +317,25 @@ func (e *EvalDeleteCommand) handleEvalArgDelete(evalID string) (int, error) {
 
 // handleFlagFilterDelete handles deletion of evaluations discovered using
 // filter flags. It is unknown how many will match the operator criteria so
-// this function ensure to batch lookup and delete requests into sensible
-// numbers.
+// this function batches lookup and delete requests into sensible numbers.
 //
 // The function is recursive and will run until it has found and deleted all
 // evals which matched the criteria. It is possible for a batch of evals to be
-// deleted, but a subsequent call to fail to due temporary issues. This is fine
-// and safe can the command can be re-run if this occurs.
+// deleted, but a subsequent call may fail due to temporary issues. The command
+// is idempotent and may be re-run safely if this occurs.
 func (e *EvalDeleteCommand) handleFlagFilterDelete(nextToken, filter string) (int, error) {
 
 	// Generate the query options using the passed next token and filter. The
 	// per page value is less than the total number we can include in a single
-	// delete request. This allows us to keep the maximum return object of the
-	// eval list call to fairly small.
+	// delete request. This keeps the maximum size of the return object at a
+	// reasonable size.
 	//
 	// The JSON serialized evaluation API object is 350-380B in size.
 	// 2426 * 380B (3.8e-4 MB) = 0.92MB. We may want to make this configurable
 	// in the future, but this is counteracted by the CLI logic which will loop
 	// until the user tells it to exit, or all evals matching the filter are
 	// deleted. 2426 * 3 falls below the maximum limit for eval IDs in a single
-	// delete request.
+	// delete request (set by MaxEvalIDsPerDeleteRequest).
 	opts := &api.QueryOptions{
 		Filter:    filter,
 		PerPage:   2426,
@@ -347,9 +344,8 @@ func (e *EvalDeleteCommand) handleFlagFilterDelete(nextToken, filter string) (in
 
 	var evalsToDelete []*api.Evaluation
 
-	// Iterate 3 times, so we have a maximum size of eval IDs to delete in a
-	// single go, but don't perform a large single eval listing in a single
-	// call.
+	// Call List 3 times to accumulate the maximum number if eval IDs supported
+	// in a single Delete request. See math above.
 	for i := 0; i < 3; i++ {
 
 		evalList, meta, err := e.client.Evaluations().List(opts)

nomad/core_sched_test.go

@@ -1915,7 +1915,6 @@ func TestCoreScheduler_PartitionDeploymentReap(t *testing.T) {
 }
 
 func TestCoreScheduler_PartitionJobReap(t *testing.T) {
-	ci.Parallel(t)
 
 	s1, cleanupS1 := TestServer(t, nil)
 	defer cleanupS1()
@@ -1929,7 +1928,11 @@ func TestCoreScheduler_PartitionJobReap(t *testing.T) {
 	core := NewCoreScheduler(s1, snap)
 
 	// Set the max ids per reap to something lower.
+	originalMaxUUIDsPerWriteRequest := structs.MaxUUIDsPerWriteRequest
 	structs.MaxUUIDsPerWriteRequest = 2
+	defer func() {
+		structs.MaxUUIDsPerWriteRequest = originalMaxUUIDsPerWriteRequest
+	}()
 
 	jobs := []*structs.Job{mock.Job(), mock.Job(), mock.Job()}
 	requests := core.(*CoreScheduler).partitionJobReap(jobs, "")

nomad/eval_endpoint.go

@@ -440,7 +440,7 @@ func (e *Eval) Delete(
 	// The eval broker must be disabled otherwise Nomad's state will likely get
 	// wild in a very un-fun way.
 	if e.srv.evalBroker.Enabled() {
-		return errors.New("eval broker is enabled")
+		return errors.New("eval broker is enabled; eval broker must be paused to delete evals")
 	}
 
 	// Grab the state snapshot, so we can look up relevant eval information.

nomad/structs/eval.go

@@ -10,7 +10,7 @@ const (
 )
 
 // EvalDeleteRequest is the request object used when operators are manually
-// deleting evaluations. The number of evaluations IDs within the request must
+// deleting evaluations. The number of evaluation IDs within the request must
 // not be greater than MaxEvalIDsPerDeleteRequest.
 type EvalDeleteRequest struct {
 	EvalIDs []string

nomad/structs/structs.go

@@ -822,9 +822,11 @@ type EvalUpdateRequest struct {
 	WriteRequest
 }
 
-// EvalReapRequest is used for reaping evaluations and allocation. This action
-// is performed by the core scheduler which is periodic, or manually by
-// operators using the eval delete RPC.
+// EvalReapRequest is used for reaping evaluations and allocation. This struct
+// is used by the Eval.Reap RPC endpoint as a request argument, and also when
+// performing eval reap or deletes via Raft. This is because Eval.Reap and
+// Eval.Delete use the same Raft message when performing deletes so we do not
+// need more Raft message types.
 type EvalReapRequest struct {
 	Evals  []string
 	Allocs []string

website/content/api-docs/evaluations.mdx

@@ -213,7 +213,7 @@ endpoint.
 
 This API endpoint should be used cautiously and only in outage situations where
 there is a large backlog of evaluations not being processed. During most normal
-and outage scenarios, Nomads reconciliation and state management will handle
+and outage scenarios, Nomad's reconciliation and state management will handle
 evaluations as needed.
 
 | Method    | Path              | Produces           |
@@ -230,8 +230,8 @@ The table below shows this endpoint's support for
 
 ### Parameters
 
-- `:EvalIDs` `(array<string>: <required>)`- An array of evaluation UUIDs to
-  delete. This must be the full UUID, not the short 8-character one.
+- `EvalIDs` `(array<string>: <required>)`- An array of evaluation UUIDs to
+  delete. This must be a full length UUID and not a prefix.
 
 ### Sample Payload
 

website/content/docs/commands/eval/delete.mdx

@@ -10,7 +10,7 @@ The eval delete command is used to delete evaluations.
 The `eval delete` command is used to delete evaluations. It should be used
 cautiously and only in outage situations where there is a large backlog of
 evaluations not being processed. During most normal and outage scenarios,
-Nomads reconciliation and state management will handle evaluations as needed.
+Nomad's reconciliation and state management will handle evaluations as needed.
 
 ## Usage
 
@@ -30,10 +30,10 @@ When ACLs are enabled, this command requires a `management` token.
 
 ## Delete Options
 
-- `-scheduler`: Filter by scheduler type (one of "batch", "service", "system",
-  or "sysbatch"). This option may be specified multiple times.
+- `-scheduler`: Filter by scheduler type (one of `batch`, `service`, `system`,
+  or `sysbatch`). This option may be specified multiple times.
 
-- `-status`: Filter by eval status (one of "blocked", "pending", "complete",
+- `-status`: Filter by eval status (one of `blocked`, `pending`, `complete`,
   "failed", or "canceled"). This option may be specified multiple times.
 
 - `-job`: Filter by an associated job ID. This option may be specified multiple
@@ -46,7 +46,7 @@ When ACLs are enabled, this command requires a `management` token.
   times.
 
 - `-filter-operator`: The filter operator to use when multiple filter options are
-  specified. This defaults to "and" but also supports "or".
+  specified. This defaults to `and` but also supports `or`.
 
 - `-yes`: Bypass the confirmation prompt if an evaluation ID was not provided.