diff --git a/.changelog/20522.txt b/.changelog/20522.txt new file mode 100644 index 00000000000..08cc08c06da --- /dev/null +++ b/.changelog/20522.txt @@ -0,0 +1,3 @@ +```release-note:bug +structs: Fix job canonicalization for array type fields +``` diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 80c0acac479..ed426c77d5f 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -4524,12 +4524,24 @@ func (j *Job) Canonicalize() { return } - // Ensure that an empty and nil map are treated the same to avoid scheduling + // Ensure that an empty and nil map or array are treated the same to avoid scheduling // problems since we use reflect DeepEquals. if len(j.Meta) == 0 { j.Meta = nil } + if len(j.Constraints) == 0 { + j.Constraints = nil + } + + if len(j.Affinities) == 0 { + j.Affinities = nil + } + + if len(j.Spreads) == 0 { + j.Spreads = nil + } + // Ensure the job is in a namespace. if j.Namespace == "" { j.Namespace = DefaultNamespace @@ -6734,12 +6746,24 @@ func (tg *TaskGroup) Copy() *TaskGroup { // Canonicalize is used to canonicalize fields in the TaskGroup. func (tg *TaskGroup) Canonicalize(job *Job) { - // Ensure that an empty and nil map are treated the same to avoid scheduling + // Ensure that an empty and nil map or array are treated the same to avoid scheduling // problems since we use reflect DeepEquals. if len(tg.Meta) == 0 { tg.Meta = nil } + if len(tg.Constraints) == 0 { + tg.Constraints = nil + } + + if len(tg.Affinities) == 0 { + tg.Affinities = nil + } + + if len(tg.Spreads) == 0 { + tg.Spreads = nil + } + // Set the default restart policy. if tg.RestartPolicy == nil { tg.RestartPolicy = NewRestartPolicy(job.Type) @@ -7704,7 +7728,7 @@ func (t *Task) Copy() *Task { // Canonicalize canonicalizes fields in the task. func (t *Task) Canonicalize(job *Job, tg *TaskGroup) { - // Ensure that an empty and nil map are treated the same to avoid scheduling + // Ensure that an empty and nil map or array are treated the same to avoid scheduling // problems since we use reflect DeepEquals. if len(t.Meta) == 0 { t.Meta = nil @@ -7715,6 +7739,17 @@ func (t *Task) Canonicalize(job *Job, tg *TaskGroup) { if len(t.Env) == 0 { t.Env = nil } + if len(t.Constraints) == 0 { + t.Constraints = nil + } + + if len(t.Affinities) == 0 { + t.Affinities = nil + } + + if len(t.VolumeMounts) == 0 { + t.VolumeMounts = nil + } for _, service := range t.Services { service.Canonicalize(job.Name, tg.Name, t.Name, job.Namespace) diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 5e3dc3984e3..d292f29cb98 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -799,6 +799,38 @@ func TestJob_Copy(t *testing.T) { } } +func TestJob_Canonicalize(t *testing.T) { + ci.Parallel(t) + cases := []struct { + job *Job + }{ + { + job: testJob(), + }, + { + job: &Job{}, + }, + { + job: &Job{ + Datacenters: []string{}, + Constraints: []*Constraint{}, + Affinities: []*Affinity{}, + Spreads: []*Spread{}, + TaskGroups: []*TaskGroup{}, + Meta: map[string]string{}, + }, + }, + } + + for _, c := range cases { + c.job.Canonicalize() + copied := c.job.Copy() + if !reflect.DeepEqual(c.job, copied) { + t.Fatalf("Canonicalize() returned a Job that changed after copy; before %#v; after %#v", c.job, copied) + } + } +} + func TestJob_IsPeriodic(t *testing.T) { ci.Parallel(t) @@ -2010,6 +2042,41 @@ func TestTaskGroupNetwork_Validate(t *testing.T) { } } +func TestTaskGroup_Canonicalize(t *testing.T) { + ci.Parallel(t) + job := testJob() + cases := []struct { + tg *TaskGroup + }{ + { + tg: job.TaskGroups[0], + }, + { + tg: &TaskGroup{}, + }, + { + tg: &TaskGroup{ + Constraints: []*Constraint{}, + Tasks: []*Task{}, + Meta: map[string]string{}, + Affinities: []*Affinity{}, + Spreads: []*Spread{}, + Networks: []*NetworkResource{}, + Services: []*Service{}, + Volumes: map[string]*VolumeRequest{}, + }, + }, + } + + for _, c := range cases { + c.tg.Canonicalize(job) + copied := c.tg.Copy() + if !reflect.DeepEqual(c.tg, copied) { + t.Fatalf("Canonicalize() returned a TaskGroup that changed after copy; before %#v; after %#v", c.tg, copied) + } + } +} + func TestTask_Validate(t *testing.T) { ci.Parallel(t) @@ -2181,6 +2248,46 @@ func TestTask_Validate_Resources(t *testing.T) { } } +func TestTask_Canonicalize(t *testing.T) { + ci.Parallel(t) + job := testJob() + tg := job.TaskGroups[0] + cases := []struct { + task *Task + }{ + { + task: tg.Tasks[0], + }, + { + task: &Task{}, + }, + { + task: &Task{ + Config: map[string]interface{}{}, + Env: map[string]string{}, + Services: []*Service{}, + Templates: []*Template{}, + Constraints: []*Constraint{}, + Affinities: []*Affinity{}, + Meta: map[string]string{}, + Artifacts: []*TaskArtifact{}, + VolumeMounts: []*VolumeMount{}, + ScalingPolicies: []*ScalingPolicy{}, + Identities: []*WorkloadIdentity{}, + Actions: []*Action{}, + }, + }, + } + + for _, c := range cases { + c.task.Canonicalize(job, tg) + copied := c.task.Copy() + if !reflect.DeepEqual(c.task, copied) { + t.Fatalf("Canonicalize() returned a Task that changed after copy; before %#v; after %#v", c.task, copied) + } + } +} + func TestNetworkResource_Copy(t *testing.T) { ci.Parallel(t)