Merge pull request #1434 from hashicorp/b-nil-empty

Sanitize empty slices/maps in Jobs

command/plan.go

@@ -96,7 +96,7 @@ func (c *PlanCommand) Run(args []string) int {
 	}
 
 	// Initialize any fields that need to be.
-	job.InitFields()
+	job.Canonicalize()
 
 	// Check that the job is valid
 	if err := job.Validate(); err != nil {

command/run.go

@@ -145,7 +145,7 @@ func (c *RunCommand) Run(args []string) int {
 	}
 
 	// Initialize any fields that need to be.
-	job.InitFields()
+	job.Canonicalize()
 
 	// Check that the job is valid
 	if err := job.Validate(); err != nil {

command/validate.go

@@ -49,7 +49,7 @@ func (c *ValidateCommand) Run(args []string) int {
 	}
 
 	// Initialize any fields that need to be.
-	job.InitFields()
+	job.Canonicalize()
 
 	// Check that the job is valid
 	if err := job.Validate(); err != nil {

nomad/fsm.go

@@ -227,6 +227,13 @@ func (n *nomadFSM) applyUpsertJob(buf []byte, index uint64) interface{} {
 		panic(fmt.Errorf("failed to decode request: %v", err))
 	}
 
+	// COMPAT: Remove in 0.5
+	// Empty maps and slices should be treated as nil to avoid
+	// un-intended destructive updates in scheduler since we use
+	// reflect.DeepEqual. Starting Nomad 0.4.1, job submission sanatizes
+	// the incoming job.
+	req.Job.Canonicalize()
+
 	if err := n.state.UpsertJob(index, req.Job); err != nil {
 		n.logger.Printf("[ERR] nomad.fsm: UpsertJob failed: %v", err)
 		return err
@@ -500,6 +507,14 @@ func (n *nomadFSM) Restore(old io.ReadCloser) error {
 			if err := dec.Decode(job); err != nil {
 				return err
 			}
+
+			// COMPAT: Remove in 0.5
+			// Empty maps and slices should be treated as nil to avoid
+			// un-intended destructive updates in scheduler since we use
+			// reflect.DeepEqual. Starting Nomad 0.4.1, job submission sanatizes
+			// the incoming job.
+			job.Canonicalize()
+
 			if err := restore.JobRestore(job); err != nil {
 				return err
 			}

nomad/job_endpoint.go

@@ -37,7 +37,7 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis
 	}
 
 	// Initialize the job fields (sets defaults and any necessary init work).
-	args.Job.InitFields()
+	args.Job.Canonicalize()
 
 	// Validate the job.
 	if err := validateJob(args.Job); err != nil {
@@ -431,7 +431,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse)
 	}
 
 	// Initialize the job fields (sets defaults and any necessary init work).
-	args.Job.InitFields()
+	args.Job.Canonicalize()
 
 	// Validate the job.
 	if err := validateJob(args.Job); err != nil {

nomad/mock/mock.go

@@ -147,7 +147,7 @@ func Job() *structs.Job {
 		ModifyIndex:    99,
 		JobModifyIndex: 99,
 	}
-	job.InitFields()
+	job.Canonicalize()
 	return job
 }
 

nomad/state/state_store_test.go

@@ -2184,7 +2184,7 @@ func TestStateStore_GetJobStatus_DeadEvalsAndAllocs(t *testing.T) {
 	// Create a mock alloc that is dead.
 	alloc := mock.Alloc()
 	alloc.JobID = job.ID
-	alloc.DesiredStatus = structs.AllocDesiredStatusFailed
+	alloc.DesiredStatus = structs.AllocDesiredStatusStop
 	if err := state.UpsertAllocs(1000, []*structs.Allocation{alloc}); err != nil {
 		t.Fatalf("err: %v", err)
 	}

nomad/structs/structs.go

@@ -736,6 +736,18 @@ func (r *Resources) Merge(other *Resources) {
 	}
 }
 
+func (r *Resources) Canonicalize() {
+	// Ensure that an empty and nil slices are treated the same to avoid scheduling
+	// problems since we use reflect DeepEquals.
+	if len(r.Networks) == 0 {
+		r.Networks = nil
+	}
+
+	for _, n := range r.Networks {
+		n.Canonicalize()
+	}
+}
+
 // MeetsMinResources returns an error if the resources specified are less than
 // the minimum allowed.
 func (r *Resources) MeetsMinResources() error {
@@ -850,6 +862,17 @@ type NetworkResource struct {
 	DynamicPorts  []Port // Dynamically assigned ports
 }
 
+func (n *NetworkResource) Canonicalize() {
+	// Ensure that an empty and nil slices are treated the same to avoid scheduling
+	// problems since we use reflect DeepEquals.
+	if len(n.ReservedPorts) == 0 {
+		n.ReservedPorts = nil
+	}
+	if len(n.DynamicPorts) == 0 {
+		n.DynamicPorts = nil
+	}
+}
+
 // MeetsMinResources returns an error if the resources specified are less than
 // the minimum allowed.
 func (n *NetworkResource) MeetsMinResources() error {
@@ -1020,11 +1043,17 @@ type Job struct {
 	JobModifyIndex uint64
 }
 
-// InitFields is used to initialize fields in the Job. This should be called
+// Canonicalize is used to canonicalize fields in the Job. This should be called
 // when registering a Job.
-func (j *Job) InitFields() {
+func (j *Job) Canonicalize() {
+	// Ensure that an empty and nil map are treated the same to avoid scheduling
+	// problems since we use reflect DeepEquals.
+	if len(j.Meta) == 0 {
+		j.Meta = nil
+	}
+
 	for _, tg := range j.TaskGroups {
-		tg.InitFields(j)
+		tg.Canonicalize(j)
 	}
 }
 
@@ -1430,15 +1459,21 @@ func (tg *TaskGroup) Copy() *TaskGroup {
 	return ntg
 }
 
-// InitFields is used to initialize fields in the TaskGroup.
-func (tg *TaskGroup) InitFields(job *Job) {
+// 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
+	// problems since we use reflect DeepEquals.
+	if len(tg.Meta) == 0 {
+		tg.Meta = nil
+	}
+
 	// Set the default restart policy.
 	if tg.RestartPolicy == nil {
 		tg.RestartPolicy = NewRestartPolicy(job.Type)
 	}
 
 	for _, task := range tg.Tasks {
-		task.InitFields(job, tg)
+		task.Canonicalize(job, tg)
 	}
 }
 
@@ -1544,6 +1579,18 @@ func (sc *ServiceCheck) Copy() *ServiceCheck {
 	return nsc
 }
 
+func (sc *ServiceCheck) Canonicalize(serviceName string) {
+	// Ensure empty slices are treated as null to avoid scheduling issues when
+	// using DeepEquals.
+	if len(sc.Args) == 0 {
+		sc.Args = nil
+	}
+
+	if sc.Name == "" {
+		sc.Name = fmt.Sprintf("service: %q check", serviceName)
+	}
+}
+
 // validate a Service's ServiceCheck
 func (sc *ServiceCheck) validate() error {
 	switch strings.ToLower(sc.Type) {
@@ -1636,9 +1683,18 @@ func (s *Service) Copy() *Service {
 	return ns
 }
 
-// InitFields interpolates values of Job, Task Group and Task in the Service
+// Canonicalize interpolates values of Job, Task Group and Task in the Service
 // Name. This also generates check names, service id and check ids.
-func (s *Service) InitFields(job string, taskGroup string, task string) {
+func (s *Service) Canonicalize(job string, taskGroup string, task string) {
+	// Ensure empty lists are treated as null to avoid scheduler issues when
+	// using DeepEquals
+	if len(s.Tags) == 0 {
+		s.Tags = nil
+	}
+	if len(s.Checks) == 0 {
+		s.Checks = nil
+	}
+
 	s.Name = args.ReplaceEnv(s.Name, map[string]string{
 		"JOB":       job,
 		"TASKGROUP": taskGroup,
@@ -1648,9 +1704,7 @@ func (s *Service) InitFields(job string, taskGroup string, task string) {
 	)
 
 	for _, check := range s.Checks {
-		if check.Name == "" {
-			check.Name = fmt.Sprintf("service: %q check", s.Name)
-		}
+		check.Canonicalize(s.Name)
 	}
 }
 
@@ -1803,9 +1857,27 @@ func (t *Task) Copy() *Task {
 	return nt
 }
 
-// InitFields initializes fields in the task.
-func (t *Task) InitFields(job *Job, tg *TaskGroup) {
-	t.InitServiceFields(job.Name, tg.Name)
+// 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
+	// problems since we use reflect DeepEquals.
+	if len(t.Meta) == 0 {
+		t.Meta = nil
+	}
+	if len(t.Config) == 0 {
+		t.Config = nil
+	}
+	if len(t.Env) == 0 {
+		t.Env = nil
+	}
+
+	for _, service := range t.Services {
+		service.Canonicalize(job.Name, tg.Name, t.Name)
+	}
+
+	if t.Resources != nil {
+		t.Resources.Canonicalize()
+	}
 
 	// Set the default timeout if it is not specified.
 	if t.KillTimeout == 0 {
@@ -1813,15 +1885,6 @@ func (t *Task) InitFields(job *Job, tg *TaskGroup) {
 	}
 }
 
-// InitServiceFields interpolates values of Job, Task Group
-// and Tasks in all the service Names of a Task. This also generates the service
-// id, check id and check names.
-func (t *Task) InitServiceFields(job string, taskGroup string) {
-	for _, service := range t.Services {
-		service.InitFields(job, taskGroup, t.Name)
-	}
-}
-
 func (t *Task) GoString() string {
 	return fmt.Sprintf("*%#v", *t)
 }

nomad/structs/structs_test.go

@@ -88,7 +88,7 @@ func TestJob_Validate(t *testing.T) {
 	if !strings.Contains(mErr.Errors[1].Error(), "group 3 missing name") {
 		t.Fatalf("err: %s", err)
 	}
-	if !strings.Contains(mErr.Errors[2].Error(), "Task group 1 validation failed") {
+	if !strings.Contains(mErr.Errors[2].Error(), "Task group web validation failed") {
 		t.Fatalf("err: %s", err)
 	}
 }
@@ -203,7 +203,7 @@ func TestJob_IsPeriodic(t *testing.T) {
 func TestJob_SystemJob_Validate(t *testing.T) {
 	j := testJob()
 	j.Type = JobTypeSystem
-	j.InitFields()
+	j.Canonicalize()
 
 	err := j.Validate()
 	if err == nil || !strings.Contains(err.Error(), "exceed") {
@@ -258,6 +258,7 @@ func TestTaskGroup_Validate(t *testing.T) {
 			Mode:     RestartPolicyModeDelay,
 		},
 	}
+
 	err = tg.Validate()
 	mErr = err.(*multierror.Error)
 	if !strings.Contains(mErr.Errors[0].Error(), "2 redefines 'web' from task 1") {
@@ -266,7 +267,7 @@ func TestTaskGroup_Validate(t *testing.T) {
 	if !strings.Contains(mErr.Errors[1].Error(), "Task 3 missing name") {
 		t.Fatalf("err: %s", err)
 	}
-	if !strings.Contains(mErr.Errors[2].Error(), "Task 1 validation failed") {
+	if !strings.Contains(mErr.Errors[2].Error(), "Task web validation failed") {
 		t.Fatalf("err: %s", err)
 	}
 }
@@ -712,7 +713,7 @@ func TestDistinctCheckID(t *testing.T) {
 
 }
 
-func TestService_InitFields(t *testing.T) {
+func TestService_Canonicalize(t *testing.T) {
 	job := "example"
 	taskGroup := "cache"
 	task := "redis"
@@ -721,25 +722,25 @@ func TestService_InitFields(t *testing.T) {
 		Name: "${TASK}-db",
 	}
 
-	s.InitFields(job, taskGroup, task)
+	s.Canonicalize(job, taskGroup, task)
 	if s.Name != "redis-db" {
 		t.Fatalf("Expected name: %v, Actual: %v", "redis-db", s.Name)
 	}
 
 	s.Name = "db"
-	s.InitFields(job, taskGroup, task)
+	s.Canonicalize(job, taskGroup, task)
 	if s.Name != "db" {
 		t.Fatalf("Expected name: %v, Actual: %v", "redis-db", s.Name)
 	}
 
 	s.Name = "${JOB}-${TASKGROUP}-${TASK}-db"
-	s.InitFields(job, taskGroup, task)
+	s.Canonicalize(job, taskGroup, task)
 	if s.Name != "example-cache-redis-db" {
 		t.Fatalf("Expected name: %v, Actual: %v", "expample-cache-redis-db", s.Name)
 	}
 
 	s.Name = "${BASE}-db"
-	s.InitFields(job, taskGroup, task)
+	s.Canonicalize(job, taskGroup, task)
 	if s.Name != "example-cache-redis-db" {
 		t.Fatalf("Expected name: %v, Actual: %v", "expample-cache-redis-db", s.Name)
 	}
@@ -777,7 +778,7 @@ func TestJob_ExpandServiceNames(t *testing.T) {
 		},
 	}
 
-	j.InitFields()
+	j.Canonicalize()
 
 	service1Name := j.TaskGroups[0].Tasks[0].Services[0].Name
 	if service1Name != "my-job-web-frontend-default" {