Skip to content

Commit

Permalink
structs: Fix job canonicalization for array type fields (#20522)
Browse files Browse the repository at this point in the history
Co-authored-by: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com>
  • Loading branch information
myszon and pkazmierczak committed May 16, 2024
1 parent 6886edf commit 898dddc
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 3 deletions.
3 changes: 3 additions & 0 deletions .changelog/20522.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
structs: Fix job canonicalization for array type fields
```
41 changes: 38 additions & 3 deletions nomad/structs/structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -4510,12 +4510,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
Expand Down Expand Up @@ -6730,12 +6742,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)
Expand Down Expand Up @@ -7794,7 +7818,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
Expand All @@ -7805,6 +7829,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)
Expand Down
107 changes: 107 additions & 0 deletions nomad/structs/structs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,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)

Expand Down Expand Up @@ -1977,6 +2009,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)

Expand Down Expand Up @@ -2148,6 +2215,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)

Expand Down

0 comments on commit 898dddc

Please sign in to comment.