api: set last index and request time on alloc stop (#16319)

Some of the methods in `Allocations()` incorrectly use the `putQuery`
in API calls where `put` is more appropriate since they are not reading
information back. These methods are also not returning request metadata
such as `LastIndex` back to callers, which can be useful to have in some
scenarios.

They also provide poor developer experience as they take an
`*api.Allocation` struct when only the allocation ID is necessary. This
can lead consumers to make unnecessary API calls to fetch the full
allocation.

Fixing these problems require updating the methods' signatures so they
take `*WriteOptions` instead of `*QueryOptions` and return `*WriteMeta`,
but this is a breaking change that requires advanced notice to consumers.

This commit adds a future breaking change notice and also fixes the
`Stop` method so it properly returns request metadata in a backwards
compatible way.

.changelog/16319.txt

@@ -0,0 +1,11 @@
+```release-note:bug
+api: Fix `Allocations().Stop()` method to properly set the request `LastIndex` and `RequestTime` in the response
+```
+
+```release-note:deprecation
+api: The `Restart()`, `Stop()`, and `Signal()` methods in the `Allocations` struct will have their signatures modified in Nomad 1.6.0
+```
+
+```release-note:deprecation
+api: The `RestartAllTasks()` method in the `Allocations` struct will be removed in Nomad 1.6.0
+```

api/allocations.go

@@ -148,6 +148,9 @@ func (a *Allocations) GC(alloc *Allocation, q *QueryOptions) error {
 // Note: for cluster topologies where API consumers don't have network access to
 // Nomad clients, set api.ClientConnTimeout to a small value (ex 1ms) to avoid
 // long pauses on this API call.
+//
+// BREAKING: This method will have the following signature in 1.6.0
+// func (a *Allocations) Restart(allocID string, taskName string, allTasks bool, w *WriteOptions) (*WriteMeta, error) {
 func (a *Allocations) Restart(alloc *Allocation, taskName string, q *QueryOptions) error {
 	req := AllocationRestartRequest{
 		TaskName: taskName,
@@ -164,6 +167,8 @@ func (a *Allocations) Restart(alloc *Allocation, taskName string, q *QueryOption
 // Note: for cluster topologies where API consumers don't have network access to
 // Nomad clients, set api.ClientConnTimeout to a small value (ex 1ms) to avoid
 // long pauses on this API call.
+//
+// DEPRECATED: This method will be removed in 1.6.0
 func (a *Allocations) RestartAllTasks(alloc *Allocation, q *QueryOptions) error {
 	req := AllocationRestartRequest{
 		AllTasks: true,
@@ -179,9 +184,29 @@ func (a *Allocations) RestartAllTasks(alloc *Allocation, q *QueryOptions) error
 // Note: for cluster topologies where API consumers don't have network access to
 // Nomad clients, set api.ClientConnTimeout to a small value (ex 1ms) to avoid
 // long pauses on this API call.
+//
+// BREAKING: This method will have the following signature in 1.6.0
+// func (a *Allocations) Stop(allocID string, w *WriteOptions) (*AllocStopResponse, error) {
 func (a *Allocations) Stop(alloc *Allocation, q *QueryOptions) (*AllocStopResponse, error) {
+	// COMPAT: Remove in 1.6.0
+	var w *WriteOptions
+	if q != nil {
+		w = &WriteOptions{
+			Region:    q.Region,
+			Namespace: q.Namespace,
+			AuthToken: q.AuthToken,
+			Headers:   q.Headers,
+			ctx:       q.ctx,
+		}
+	}
+
 	var resp AllocStopResponse
-	_, err := a.client.putQuery("/v1/allocation/"+alloc.ID+"/stop", nil, &resp, q)
+	wm, err := a.client.put("/v1/allocation/"+alloc.ID+"/stop", nil, &resp, w)
+	if wm != nil {
+		resp.LastIndex = wm.LastIndex
+		resp.RequestTime = wm.RequestTime
+	}
+
 	return &resp, err
 }
 
@@ -198,6 +223,9 @@ type AllocStopResponse struct {
 // Note: for cluster topologies where API consumers don't have network access to
 // Nomad clients, set api.ClientConnTimeout to a small value (ex 1ms) to avoid
 // long pauses on this API call.
+//
+// BREAKING: This method will have the following signature in 1.6.0
+// func (a *Allocations) Signal(allocID string, task string, signal string, w *WriteOptions) (*WriteMeta, error) {
 func (a *Allocations) Signal(alloc *Allocation, q *QueryOptions, task, signal string) error {
 	req := AllocSignalRequest{
 		Signal: signal,

api/allocations_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/nomad/api/internal/testutil"
+	"github.com/shoenig/test"
 	"github.com/shoenig/test/must"
 )
 
@@ -271,6 +272,40 @@ func TestAllocations_RescheduleInfo(t *testing.T) {
 
 }
 
+func TestAllocations_Stop(t *testing.T) {
+	testutil.RequireRoot(t)
+	testutil.Parallel(t)
+
+	c, s := makeClient(t, nil, func(c *testutil.TestServerConfig) {
+		c.DevMode = true
+	})
+	defer s.Stop()
+	a := c.Allocations()
+
+	// wait for node
+	_ = oneNodeFromNodeList(t, c.Nodes())
+
+	// Create a job and register it
+	job := testJob()
+	_, wm, err := c.Jobs().Register(job, nil)
+	must.NoError(t, err)
+
+	// List allocations.
+	stubs, qm, err := a.List(&QueryOptions{WaitIndex: wm.LastIndex})
+	must.NoError(t, err)
+	must.SliceLen(t, 1, stubs)
+
+	// Stop the first allocation.
+	resp, err := a.Stop(&Allocation{ID: stubs[0].ID}, &QueryOptions{WaitIndex: qm.LastIndex})
+	must.NoError(t, err)
+	test.UUIDv4(t, resp.EvalID)
+	test.NonZero(t, resp.LastIndex)
+
+	// Stop allocation that doesn't exist.
+	resp, err = a.Stop(&Allocation{ID: "invalid"}, &QueryOptions{WaitIndex: qm.LastIndex})
+	must.Error(t, err)
+}
+
 // TestAllocations_ExecErrors ensures errors are properly formatted
 func TestAllocations_ExecErrors(t *testing.T) {
 	testutil.Parallel(t)