Skip to content

Commit

Permalink
Only initialize task.VolumeMounts when not-nil (#10990)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Mahmood Ali committed Aug 26, 2021
1 parent de955b7 commit f8c414b
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 19 deletions.
3 changes: 3 additions & 0 deletions .changelog/10990.txt
Original file line number Diff line number Diff line change
@@ -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
```
22 changes: 12 additions & 10 deletions command/agent/job_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -1047,16 +1047,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,
})
}
}
}

Expand Down
27 changes: 18 additions & 9 deletions command/agent/job_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@ import (
"net/http"
"net/http/httptest"
"reflect"
"strings"
"testing"
"time"

"github.com/golang/snappy"
"github.com/kr/pretty"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -2144,6 +2142,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),
Expand Down Expand Up @@ -2526,6 +2532,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,
Expand Down Expand Up @@ -2684,9 +2698,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),
Expand Down Expand Up @@ -2928,10 +2940,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) {
Expand Down

0 comments on commit f8c414b

Please sign in to comment.