apply more code review changes

Author: lgfa29

.changelog/15068.txt

@@ -1,7 +1,7 @@
 ```release-note:bug
-scheduler: Fixed a bug that prevented allocations with `max_client_disconnect` set to be updated after they reconnected.
+scheduler: Fixed a bug that prevented disconnected allocations to be updated after they reconnect.
 ```
 
 ```release-note:bug
-scheduler: Prevent unnecessary placements when allocations with `max_client_disconnect` set reconnect.
+scheduler: Prevent unnecessary placements when disconnected allocations reconnect.
 ```

nomad/node_endpoint.go

@@ -1132,7 +1132,9 @@ func (n *Node) GetClientAllocs(args *structs.NodeSpecificRequest,
 	return n.srv.blockingRPC(&opts)
 }
 
-// UpdateAlloc is used to update the client status of an allocation
+// UpdateAlloc is used to update the client status of an allocation. It can
+// only be called by clients.
+//
 // Clients must first register and heartbeat successfully before being able to
 // call this method.
 func (n *Node) UpdateAlloc(args *structs.AllocUpdateRequest, reply *structs.GenericResponse) error {
@@ -1161,6 +1163,10 @@ func (n *Node) UpdateAlloc(args *structs.AllocUpdateRequest, reply *structs.Gene
 		// NodeID field in the request.
 		nodeID = args.Alloc[0].NodeID
 	}
+	if nodeID == "" {
+		return fmt.Errorf("missing node ID")
+	}
+
 	node, err := n.srv.State().NodeByID(nil, nodeID)
 	if err != nil {
 		return fmt.Errorf("failed to retrieve node %s: %v", args.NodeID, err)

scheduler/reconcile.go

@@ -1203,9 +1203,10 @@ func (a *allocReconciler) computeReconnecting(reconnecting allocSet) {
 
 		// Record the new ClientStatus to indicate to future evals that the
 		// alloc has already reconnected.
-		alloc.AppendState(structs.AllocStateFieldClientStatus, alloc.ClientStatus)
-
-		a.result.reconnectUpdates[alloc.ID] = alloc
+		// Use a copy to prevent mutating the object from statestore.
+		reconnectedAlloc := alloc.Copy()
+		reconnectedAlloc.AppendState(structs.AllocStateFieldClientStatus, alloc.ClientStatus)
+		a.result.reconnectUpdates[alloc.ID] = reconnectedAlloc
 	}
 }
 

scheduler/reconcile_util.go

@@ -240,7 +240,7 @@ func (a allocSet) filterByTainted(taintedNodes map[string]*structs.Node, serverS
 				alloc.ClientStatus == structs.AllocClientStatusFailed) {
 			reconnect = alloc.NeedsToReconnect()
 			if reconnect {
-				expired = alloc.Expired(time.Now())
+				expired = alloc.Expired(now)
 			}
 		}