scheduler: prevent spurious placement on reconnect

When a client reconnects it makes two independent RPC calls:

  - `Node.UpdateStatus` to heartbeat and set its status as `ready`.
  - `Node.UpdateAlloc` to update the status of its allocations.

These two calls can happen in any order, and in case the allocations are
updated before a heartbeat it causes the state to be the same as a node
being disconnected: the node status will still be `disconnected` while
the allocation `ClientStatus` is set to `running`.

The current implementation did not handle this order of events properly,
and the scheduler would create an unnecessary placement since it
considered the allocation was being disconnected. This extra allocation
would then be quickly stopped by the heartbeat eval.

This commit adds a new code path to handle this order of events. If the
node is `disconnected` and the allocation `ClientStatus` is `running`
the scheduler will check if the allocation is actually reconnecting
using its `AllocState` events.

Author: lgfa29

scheduler/reconcile_test.go

@@ -5608,6 +5608,35 @@ func TestReconciler_Disconnected_Client(t *testing.T) {
 				},
 			},
 		},
+		{
+			// This case tests when the disconnected client reconnects but
+			// sends a Node.UpdateAlloc RPC before a Node.UpdateStatus
+			// heartbeat, setting the ClientStatus for the previously
+			// disconnected allocs to "running" before the node status is set
+			// to "ready".
+			name:                    "reconnect-with-alloc-update-before-heartbeat",
+			allocCount:              5,
+			replace:                 true,
+			disconnectedAllocCount:  2,
+			disconnectedAllocStatus: structs.AllocClientStatusRunning,
+			disconnectedAllocStates: []*structs.AllocState{{
+				Field: structs.AllocStateFieldClientStatus,
+				Value: structs.AllocClientStatusUnknown,
+				Time:  time.Now(),
+			}},
+			serverDesiredStatus:    structs.AllocDesiredStatusRun,
+			nodeStatusDisconnected: true,
+			expected: &resultExpectation{
+				stop:             2,
+				reconnectUpdates: 2,
+				desiredTGUpdates: map[string]*structs.DesiredUpdates{
+					"web": {
+						Stop:   2, // stop the 2 replacements
+						Ignore: 5, // ignore other allocs since they are all still running
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {

scheduler/reconcile_util.go

@@ -262,6 +262,15 @@ func (a allocSet) filterByTainted(taintedNodes map[string]*structs.Node, serverS
 				if supportsDisconnectedClients {
 					// Filter running allocs on a node that is disconnected to be marked as unknown.
 					if alloc.ClientStatus == structs.AllocClientStatusRunning {
+						// Handle the case where the client updates its allocs
+						// before sending a heartbeat. The alloc will be set to
+						// running, but the client is still considered to be
+						// disconnected.
+						if reconnect {
+							reconnecting[alloc.ID] = alloc
+							continue
+						}
+
 						disconnecting[alloc.ID] = alloc
 						continue
 					}

scheduler/reconcile_util_test.go

@@ -783,6 +783,69 @@ func TestAllocSet_filterByTainted(t *testing.T) {
 			ignore: allocSet{},
 			lost:   allocSet{},
 		},
+		{
+			// On reconnect the node may send a Node.UpdateAlloc RPC before it
+			// heartbeats, so the alloc ClientStatus changes from "unknown" to
+			// "running" before the client itself is marked as "ready".
+			// This scenario is similar to a client disconnecting for the first
+			// time, but we _should not_ create a replacement when the client
+			// reconnects.
+			name:                        "disco-client-no-placement-on-reconnect",
+			supportsDisconnectedClients: true,
+			now:                         time.Now(),
+			taintedNodes:                nodes,
+			skipNilNodeTest:             false,
+			all: allocSet{
+				"running-replacement": {
+					ID:                 "running-replacement",
+					Name:               "web",
+					ClientStatus:       structs.AllocClientStatusRunning,
+					DesiredStatus:      structs.AllocDesiredStatusRun,
+					Job:                testJob,
+					NodeID:             "normal",
+					TaskGroup:          "web",
+					PreviousAllocation: "running-original",
+				},
+				"running-original": {
+					ID:            "running-original",
+					Name:          "web",
+					ClientStatus:  structs.AllocClientStatusRunning,
+					DesiredStatus: structs.AllocDesiredStatusRun,
+					Job:           testJob,
+					NodeID:        "disconnected",
+					TaskGroup:     "web",
+					AllocStates:   unknownAllocState,
+				},
+			},
+			untainted: allocSet{
+				"running-replacement": {
+					ID:                 "running-replacement",
+					Name:               "web",
+					ClientStatus:       structs.AllocClientStatusRunning,
+					DesiredStatus:      structs.AllocDesiredStatusRun,
+					Job:                testJob,
+					NodeID:             "normal",
+					TaskGroup:          "web",
+					PreviousAllocation: "running-original",
+				},
+			},
+			migrate:       allocSet{},
+			disconnecting: allocSet{},
+			reconnecting: allocSet{
+				"running-original": {
+					ID:            "running-original",
+					Name:          "web",
+					ClientStatus:  structs.AllocClientStatusRunning,
+					DesiredStatus: structs.AllocDesiredStatusRun,
+					Job:           testJob,
+					NodeID:        "disconnected",
+					TaskGroup:     "web",
+					AllocStates:   unknownAllocState,
+				},
+			},
+			ignore: allocSet{},
+			lost:   allocSet{},
+		},
 		{
 			// After an alloc is reconnected, it should be considered
 			// "untainted" instead of "reconnecting" to allow changes such as

scheduler/util.go

@@ -195,6 +195,20 @@ func diffSystemAllocsForNode(
 				node.Status == structs.NodeStatusDisconnected &&
 				exist.ClientStatus == structs.AllocClientStatusRunning {
 
+				// Handle the case where the client updates its allocs before
+				// sending a heartbeat. The alloc will be set to running, but
+				// the client is still considered to be disconnected.
+				if reconnect {
+					reconnecting := exist.Copy()
+					reconnecting.AppendState(structs.AllocStateFieldClientStatus, exist.ClientStatus)
+					result.reconnecting = append(result.reconnecting, allocTuple{
+						Name:      name,
+						TaskGroup: tg,
+						Alloc:     reconnecting,
+					})
+					continue
+				}
+
 				disconnect := exist.Copy()
 				disconnect.ClientStatus = structs.AllocClientStatusUnknown
 				disconnect.AppendState(structs.AllocStateFieldClientStatus, structs.AllocClientStatusUnknown)

scheduler/util_test.go

@@ -355,6 +355,22 @@ func TestDiffSystemAllocsForNode_DisconnectedNode(t *testing.T) {
 				reconnecting: 1,
 			},
 		},
+		{
+			name: "disconnected alloc reconnects even when AllocUpdate happens before heartbeat",
+			node: disconnectedNode,
+			allocFn: func(alloc *structs.Allocation) {
+				alloc.ClientStatus = structs.AllocClientStatusRunning
+
+				alloc.AllocStates = []*structs.AllocState{{
+					Field: structs.AllocStateFieldClientStatus,
+					Value: structs.AllocClientStatusUnknown,
+					Time:  time.Now().Add(-time.Minute),
+				}}
+			},
+			expect: diffResultCount{
+				reconnecting: 1,
+			},
+		},
 		{
 			name: "alloc not reconnecting after it reconnects",
 			node: readyNode,