From 194f34a0ac54fdea6d6869cf3f846fcb100221e6 Mon Sep 17 00:00:00 2001
From: Drew Bailey <drewbailey5@gmail.com>
Date: Fri, 8 Jan 2021 09:00:41 -0500
Subject: [PATCH] persist shared ports during inplace updates (#9736)

AllocatedSharedResources were not being copied over to the new
allocation struct the scheduler makes during inplace updates. This
caused downstream issues after the plan was applied, namely the shared
ports were dropped causing issues with service
registration/deregistration.

test that shared ports are preserved

change log, also carry over shared network

copy networks
---
 CHANGELOG.md           |  1 +
 scheduler/util.go      |  4 ++-
 scheduler/util_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9b0a810c68f..8401d8e3e0f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,7 @@ IMPROVEMENTS:
 
 BUG FIXES:
  * client: Fixed a bug where clients configured with `cpu_total_compute` did not update the `cpu.totalcompute` node attribute. [[GH-9532](https://github.com/hashicorp/nomad/issues/9532)]
+ * core: Fixed a bug where an in place update dropped an allocations shared allocated resources [[GH-9736](https://github.com/hashicorp/nomad/issues/9736)]
  * consul: Fixed a bug where updating a task to include services would not work [[GH-9707](https://github.com/hashicorp/nomad/issues/9707)]
  * consul: Fixed alloc address mode port advertisement to use the mapped `to` port value [[GH-9730](https://github.com/hashicorp/nomad/issues/9730)]
  * consul/connect: Fixed a bug where absent ingress envoy proxy configuration could panic client [[GH-9669](https://github.com/hashicorp/nomad/issues/9669)]
diff --git a/scheduler/util.go b/scheduler/util.go
index 7261f67deb8..b42895a16b8 100644
--- a/scheduler/util.go
+++ b/scheduler/util.go
@@ -728,7 +728,9 @@ func inplaceUpdate(ctx Context, eval *structs.Evaluation, job *structs.Job,
 			Tasks:          option.TaskResources,
 			TaskLifecycles: option.TaskLifecycles,
 			Shared: structs.AllocatedSharedResources{
-				DiskMB: int64(update.TaskGroup.EphemeralDisk.SizeMB),
+				DiskMB:   int64(update.TaskGroup.EphemeralDisk.SizeMB),
+				Ports:    update.Alloc.AllocatedResources.Shared.Ports,
+				Networks: update.Alloc.AllocatedResources.Shared.Networks.Copy(),
 			},
 		}
 		newAlloc.Metrics = ctx.Metrics()
diff --git a/scheduler/util_test.go b/scheduler/util_test.go
index 654b23f2424..7ca5255e67c 100644
--- a/scheduler/util_test.go
+++ b/scheduler/util_test.go
@@ -909,6 +909,63 @@ func TestInplaceUpdate_ChangedTaskGroup(t *testing.T) {
 	require.Empty(t, ctx.plan.NodeAllocation, "inplaceUpdate incorrectly did an inplace update")
 }
 
+func TestInplaceUpdate_AllocatedResources(t *testing.T) {
+	state, ctx := testContext(t)
+	eval := mock.Eval()
+	job := mock.Job()
+
+	node := mock.Node()
+	require.NoError(t, state.UpsertNode(structs.MsgTypeTestSetup, 900, node))
+
+	// Register an alloc
+	alloc := &structs.Allocation{
+		Namespace: structs.DefaultNamespace,
+		ID:        uuid.Generate(),
+		EvalID:    eval.ID,
+		NodeID:    node.ID,
+		JobID:     job.ID,
+		Job:       job,
+		AllocatedResources: &structs.AllocatedResources{
+			Shared: structs.AllocatedSharedResources{
+				Ports: structs.AllocatedPorts{
+					{
+						Label: "api-port",
+						Value: 19910,
+						To:    8080,
+					},
+				},
+			},
+		},
+		DesiredStatus: structs.AllocDesiredStatusRun,
+		TaskGroup:     "web",
+	}
+	alloc.TaskResources = map[string]*structs.Resources{"web": alloc.Resources}
+	require.NoError(t, state.UpsertJobSummary(1000, mock.JobSummary(alloc.JobID)))
+	require.NoError(t, state.UpsertAllocs(structs.MsgTypeTestSetup, 1001, []*structs.Allocation{alloc}))
+
+	// Update TG to add a new service (inplace)
+	tg := job.TaskGroups[0]
+	tg.Services = []*structs.Service{
+		{
+			Name: "tg-service",
+		},
+	}
+
+	updates := []allocTuple{{Alloc: alloc, TaskGroup: tg}}
+	stack := NewGenericStack(false, ctx)
+
+	// Do the inplace update.
+	unplaced, inplace := inplaceUpdate(ctx, eval, job, stack, updates)
+
+	require.True(t, len(unplaced) == 0 && len(inplace) == 1, "inplaceUpdate incorrectly did not perform an inplace update")
+	require.NotEmpty(t, ctx.plan.NodeAllocation, "inplaceUpdate incorrectly did an inplace update")
+	require.NotEmpty(t, ctx.plan.NodeAllocation[node.ID][0].AllocatedResources.Shared.Ports)
+
+	port, ok := ctx.plan.NodeAllocation[node.ID][0].AllocatedResources.Shared.Ports.Get("api-port")
+	require.True(t, ok)
+	require.Equal(t, 19910, port.Value)
+}
+
 func TestInplaceUpdate_NoMatch(t *testing.T) {
 	state, ctx := testContext(t)
 	eval := mock.Eval()