Code review feedback

command/agent/job_endpoint.go

@@ -92,8 +92,8 @@ func (s *HTTPServer) jobForceEvaluate(resp http.ResponseWriter, req *http.Reques
 	}
 	var args structs.JobEvaluateRequest
 
-	// TODO(preetha) remove in 0.9
+	// TODO(preetha): remove in 0.9
-	// For backwards compatibility allow using this endpoint without a payload
+	// COMPAT: For backwards compatibility allow using this endpoint without a payload
 	if req.ContentLength == 0 {
 		args = structs.JobEvaluateRequest{
 			JobID: jobName,

command/job_eval.go

@@ -18,7 +18,9 @@ func (c *JobEvalCommand) Help() string {
 	helpText := `
 Usage: nomad job eval [options] <job_id>
 
-  Force an evaluation of the provided job id
+  Force an evaluation of the provided job ID. Forcing an evaluation will trigger the scheduler
+  to re-evaluate the job. The force flags allow operators to force the scheduler to create
+  new allocations under certain scenarios.
 
 General Options:
 
@@ -27,14 +29,14 @@ General Options:
 Eval Options:
 
   -force-reschedule
-    Force reschedule any failed allocations even if they are not currently
+    Force reschedule failed allocations even if they are not currently
-    eligible for rescheduling
+    eligible for rescheduling.
 `
 	return strings.TrimSpace(helpText)
 }
 
 func (c *JobEvalCommand) Synopsis() string {
-	return "Force evaluating a job using its job id"
+	return "Force an evaluation for the job using its job ID"
 }
 
 func (c *JobEvalCommand) AutocompleteFlags() complete.Flags {
@@ -59,7 +61,7 @@ func (c *JobEvalCommand) AutocompleteArgs() complete.Predictor {
 	})
 }
 
-func (c *JobEvalCommand) Name() string { return "eval" }
+func (c *JobEvalCommand) Name() string { return "job eval" }
 
 func (c *JobEvalCommand) Run(args []string) int {
 	flags := c.Meta.FlagSet(c.Name(), FlagSetClient)
@@ -72,8 +74,8 @@ func (c *JobEvalCommand) Run(args []string) int {
 
 	// Check that we either got no jobs or exactly one.
 	args = flags.Args()
-	if len(args) > 1 {
+	if len(args) != 1 {
-		c.Ui.Error("This command takes either no arguments or one: <job>")
+		c.Ui.Error("This command takes one argument: <job>")
 		c.Ui.Error(commandErrorText(c))
 		return 1
 	}
@@ -85,12 +87,7 @@ func (c *JobEvalCommand) Run(args []string) int {
 		return 1
 	}
 
-	if len(args) == 0 {
+	// Call eval endpoint
-		c.Ui.Error("Must provide a job ID")
-		return 1
-	}
-
-	// Call eval end point
 	jobID := args[0]
 
 	opts := api.EvalOptions{

command/job_eval_test.go

@@ -4,10 +4,15 @@ import (
 	"strings"
 	"testing"
 
+	"fmt"
+
 	"github.com/hashicorp/nomad/nomad/mock"
+	"github.com/hashicorp/nomad/nomad/structs"
+	"github.com/hashicorp/nomad/testutil"
 	"github.com/mitchellh/cli"
 	"github.com/posener/complete"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestJobEvalCommand_Implements(t *testing.T) {
@@ -33,13 +38,70 @@ func TestJobEvalCommand_Fails(t *testing.T) {
 	if code := cmd.Run([]string{}); code != 1 {
 		t.Fatalf("expect exit 1, got: %d", code)
 	}
-	if out := ui.ErrorWriter.String(); !strings.Contains(out, "Must provide a job ID") {
+	if out := ui.ErrorWriter.String(); !strings.Contains(out, "This command takes one argument") {
 		t.Fatalf("unexpected error: %v", out)
 	}
 	ui.ErrorWriter.Reset()
 
 }
 
+func TestJobEvalCommand_Run(t *testing.T) {
+	t.Parallel()
+	srv, client, url := testServer(t, true, nil)
+	defer srv.Shutdown()
+
+	// Wait for a node to be ready
+	testutil.WaitForResult(func() (bool, error) {
+		nodes, _, err := client.Nodes().List(nil)
+		if err != nil {
+			return false, err
+		}
+		for _, node := range nodes {
+			if node.Status == structs.NodeStatusReady {
+				return true, nil
+			}
+		}
+		return false, fmt.Errorf("no ready nodes")
+	}, func(err error) {
+		t.Fatalf("err: %v", err)
+	})
+
+	ui := new(cli.MockUi)
+	cmd := &JobEvalCommand{Meta: Meta{Ui: ui}}
+	require := require.New(t)
+
+	state := srv.Agent.Server().State()
+
+	// Create a job
+	job := mock.Job()
+	err := state.UpsertJob(11, job)
+	require.Nil(err)
+
+	job, err = state.JobByID(nil, structs.DefaultNamespace, job.ID)
+	require.Nil(err)
+
+	// Create a failed alloc for the job
+	alloc := mock.Alloc()
+	alloc.Job = job
+	alloc.JobID = job.ID
+	alloc.TaskGroup = job.TaskGroups[0].Name
+	alloc.Namespace = job.Namespace
+	alloc.ClientStatus = structs.AllocClientStatusFailed
+	err = state.UpsertAllocs(12, []*structs.Allocation{alloc})
+	require.Nil(err)
+
+	if code := cmd.Run([]string{"-address=" + url, "-force-reschedule", job.ID}); code != 0 {
+		t.Fatalf("expected exit 0, got: %d", code)
+	}
+
+	// Lookup alloc again
+	alloc, err = state.AllocByID(nil, alloc.ID)
+	require.NotNil(alloc)
+	require.Nil(err)
+	require.True(*alloc.DesiredTransition.ForceReschedule)
+
+}
+
 func TestJobEvalCommand_AutocompleteArgs(t *testing.T) {
 	assert := assert.New(t)
 	t.Parallel()

nomad/job_endpoint.go

@@ -557,11 +557,11 @@ func (j *Job) Evaluate(args *structs.JobEvaluateRequest, reply *structs.JobRegis
 		for _, alloc := range allocs {
 			taskGroup := job.LookupTaskGroup(alloc.TaskGroup)
 			// Forcing rescheduling is only allowed if task group has rescheduling enabled
-			if taskGroup == nil || taskGroup.ReschedulePolicy == nil || !taskGroup.ReschedulePolicy.Enabled() {
+			if taskGroup == nil || !taskGroup.ReschedulePolicy.Enabled() {
 				continue
 			}
 
-			if alloc.NextAllocation == "" && alloc.ClientStatus == structs.AllocClientStatusFailed {
+			if alloc.NextAllocation == "" && alloc.ClientStatus == structs.AllocClientStatusFailed && !alloc.DesiredTransition.ShouldForceReschedule() {
 				forceRescheduleAllocs[alloc.ID] = allowForceRescheduleTransition
 			}
 		}

website/source/api/jobs.html.md

@@ -1385,7 +1385,7 @@ The table below shows this endpoint's support for
 - `JobID` `(string: <required>)` - Specify the ID of the job in the JSON payload
 
 - `EvalOptions` `(<optional>)` - Specify additional options to be used during the forced evaluation.
-    - `ForceReschedule` `(bool: false)` - If set, any failed allocations of the job are rescheduled
+    - `ForceReschedule` `(bool: false)` - If set, failed allocations of the job are rescheduled
     immediately. This is useful for operators to force immediate placement even if the failed allocations are past
     their reschedule limit, or are delayed by several hours because the allocation's reschedule policy has exponential delay.
 
@@ -1405,7 +1405,7 @@ The table below shows this endpoint's support for
 ```text
 $ curl \
     --request POST \
-    -d@sample.json \
+    -d @sample.json \
     https://localhost:4646/v1/job/my-job/evaluate
 ```
 

website/source/docs/commands/job/eval.html.md.erb

@@ -13,7 +13,7 @@ The `job eval` command is used to force an evaluation of a job, given the job ID
 ## Usage
 
 ```
-nomad job eval [options] <jobID>
+nomad job eval [options] <job_id>
 ```
 
 The `job eval` command requires a single argument, specifying the job ID to evaluate.
@@ -26,8 +26,8 @@ the job will be evaluated, forcing a scheduler run.
 
 ## Eval Options
 
-* `-force-reschedule`: `force-reschedule` is used to force placement of any failed allocations.
+* `-force-reschedule`: `force-reschedule` is used to force placement of failed allocations.
-If this is set, failed allocations that are past their reschedule limit, as well as any that are
+If this is set, failed allocations that are past their reschedule limit, and those that are
 scheduled to be replaced at a future time are placed immediately. This option only places failed
 allocations if the task group has rescheduling enabled.