From 2bbe7b891f4442bcb27583a72d846d944f53134f Mon Sep 17 00:00:00 2001
From: Mahmood Ali <mahmood@hashicorp.com>
Date: Mon, 2 Aug 2021 13:08:10 -0400
Subject: [PATCH] Only initialize task.VolumeMounts when not-nil (#10990)

1.1.3 had a bug where task.VolumeMounts will be an empty slice instead of nil. Eventually, it gets canonicalized and is set to `nil`, but it seems to confuse dry-run planning.

The regression was introduced in https://github.com/hashicorp/nomad/pull/10855/files#diff-56b3c82fcbc857f8fb93a903f1610f6e6859b3610a4eddf92bad9ea27fdc85ecL1028-R1037 . Curiously, it's the only place where `len(apiTask.VolumeMounts)` check was dropped. I assume it was dropped accidentally.

Fixes #10981
---
 .changelog/10990.txt               |  3 +++
 command/agent/job_endpoint.go      | 22 ++++++++++---------
 command/agent/job_endpoint_test.go | 34 +++++++++++++++++++-----------
 3 files changed, 37 insertions(+), 22 deletions(-)
 create mode 100644 .changelog/10990.txt

diff --git a/.changelog/10990.txt b/.changelog/10990.txt
new file mode 100644
index 000000000000..5ce9b97e2f1c
--- /dev/null
+++ b/.changelog/10990.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+server: Fixed a bug where planning job update reports spurious in-place updates even if the update includes no changes
+```
diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go
index 5feb8ac17c40..9e45467c39e6 100644
--- a/command/agent/job_endpoint.go
+++ b/command/agent/job_endpoint.go
@@ -1044,16 +1044,18 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup,
 		}
 	}
 
-	structsTask.VolumeMounts = []*structs.VolumeMount{}
-	for _, mount := range apiTask.VolumeMounts {
-		if mount != nil && mount.Volume != nil {
-			structsTask.VolumeMounts = append(structsTask.VolumeMounts,
-				&structs.VolumeMount{
-					Volume:          *mount.Volume,
-					Destination:     *mount.Destination,
-					ReadOnly:        *mount.ReadOnly,
-					PropagationMode: *mount.PropagationMode,
-				})
+	if len(apiTask.VolumeMounts) > 0 {
+		structsTask.VolumeMounts = []*structs.VolumeMount{}
+		for _, mount := range apiTask.VolumeMounts {
+			if mount != nil && mount.Volume != nil {
+				structsTask.VolumeMounts = append(structsTask.VolumeMounts,
+					&structs.VolumeMount{
+						Volume:          *mount.Volume,
+						Destination:     *mount.Destination,
+						ReadOnly:        *mount.ReadOnly,
+						PropagationMode: *mount.PropagationMode,
+					})
+			}
 		}
 	}
 
diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go
index fe1b7155c560..aaf5ce21e6c0 100644
--- a/command/agent/job_endpoint_test.go
+++ b/command/agent/job_endpoint_test.go
@@ -4,18 +4,17 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"reflect"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/golang/snappy"
-	"github.com/hashicorp/nomad/api"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	api "github.com/hashicorp/nomad/api"
 	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
-	"github.com/kr/pretty"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestHTTP_JobsList(t *testing.T) {
@@ -2136,6 +2135,14 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 								Weight:  helper.Int8ToPtr(50),
 							},
 						},
+						VolumeMounts: []*api.VolumeMount{
+							{
+								Volume:          helper.StringToPtr("vol"),
+								Destination:     helper.StringToPtr("dest"),
+								ReadOnly:        helper.BoolToPtr(false),
+								PropagationMode: helper.StringToPtr("a"),
+							},
+						},
 						RestartPolicy: &api.RestartPolicy{
 							Interval: helper.TimeToPtr(2 * time.Second),
 							Attempts: helper.IntToPtr(10),
@@ -2510,6 +2517,14 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 						Env: map[string]string{
 							"hello": "world",
 						},
+						VolumeMounts: []*structs.VolumeMount{
+							{
+								Volume:          "vol",
+								Destination:     "dest",
+								ReadOnly:        false,
+								PropagationMode: "a",
+							},
+						},
 						RestartPolicy: &structs.RestartPolicy{
 							Interval: 2 * time.Second,
 							Attempts: 10,
@@ -2665,9 +2680,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 
 	structsJob := ApiJobToStructJob(apiJob)
 
-	if diff := pretty.Diff(expected, structsJob); len(diff) > 0 {
-		t.Fatalf("bad:\n%s", strings.Join(diff, "\n"))
-	}
+	require.Equal(t, expected, structsJob)
 
 	systemAPIJob := &api.Job{
 		Stop:        helper.BoolToPtr(true),
@@ -2903,10 +2916,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
 	}
 
 	systemStructsJob := ApiJobToStructJob(systemAPIJob)
-
-	if diff := pretty.Diff(expectedSystemJob, systemStructsJob); len(diff) > 0 {
-		t.Fatalf("bad:\n%s", strings.Join(diff, "\n"))
-	}
+	require.Equal(t, expectedSystemJob, systemStructsJob)
 }
 
 func TestJobs_ApiJobToStructsJobUpdate(t *testing.T) {