Fix switching diff structures

nomad/job_endpoint.go

@@ -460,8 +460,14 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse)
 
 	// Annotate and store the diff
 	if args.Diff {
-		jobDiff := structs.NewJobDiff(oldJob, args.Job)
-		scheduler.Annotate(jobDiff)
+		jobDiff, err := oldJob.Diff(args.Job, true)
+		if err != nil {
+			return fmt.Errorf("failed to create job diff: %v", err)
+		}
+
+		if err := scheduler.Annotate(jobDiff, planner.Plan); err != nil {
+			return fmt.Errorf("failed to annotate job diff: %v", err)
+		}
 		reply.Diff = jobDiff
 	}
 

nomad/structs/diff.go

@@ -10,26 +10,6 @@ import (
 	"github.com/mitchellh/hashstructure"
 )
 
-const (
-	// AnnotationForcesDestructiveUpdate marks a diff as causing a destructive
-	// update.
-	AnnotationForcesDestructiveUpdate = "forces create/destroy update"
-
-	// AnnotationForcesInplaceUpdate marks a diff as causing an in-place
-	// update.
-	AnnotationForcesInplaceUpdate = "forces in-place update"
-)
-
-// UpdateTypes denote the type of update to occur against the task group.
-const (
-	UpdateTypeIgnore            = "ignore"
-	UpdateTypeCreate            = "create"
-	UpdateTypeDestroy           = "destroy"
-	UpdateTypeMigrate           = "migrate"
-	UpdateTypeInplaceUpdate     = "in-place update"
-	UpdateTypeDestructiveUpdate = "create/destroy update"
-)
-
 // DiffType denotes the type of a diff object.
 type DiffType string
 
@@ -171,7 +151,7 @@ type TaskGroupDiff struct {
 	Fields  []*FieldDiff
 	Objects []*ObjectDiff
 	Tasks   []*TaskDiff
-	Updates map[string]int
+	Updates map[string]uint64
 }
 
 // Diff returns a diff of two task groups. If contextual diff is enabled,
@@ -847,9 +827,10 @@ func (o ObjectDiffs) Swap(i, j int)      { o[i], o[j] = o[j], o[i] }
 func (o ObjectDiffs) Less(i, j int) bool { return o[i].Less(o[j]) }
 
 type FieldDiff struct {
-	Type     DiffType
-	Name     string
-	Old, New string
+	Type        DiffType
+	Name        string
+	Old, New    string
+	Annotations []string
 }
 
 // fieldDiff returns a FieldDiff if old and new are different otherwise, it
@@ -880,7 +861,12 @@ func fieldDiff(old, new, name string, contextual bool) *FieldDiff {
 }
 
 func (f *FieldDiff) GoString() string {
-	return fmt.Sprintf("%q (%s): %q => %q", f.Name, f.Type, f.Old, f.New)
+	out := fmt.Sprintf("%q (%s): %q => %q", f.Name, f.Type, f.Old, f.New)
+	if len(f.Annotations) != 0 {
+		out += fmt.Sprintf(" (%s)", strings.Join(f.Annotations, ", "))
+	}
+
+	return out
 }
 
 func (f *FieldDiff) Less(other *FieldDiff) bool {

scheduler/annotate.go

@@ -1,6 +1,11 @@
 package scheduler
 
-import "github.com/hashicorp/nomad/nomad/structs"
+import (
+	"fmt"
+	"strconv"
+
+	"github.com/hashicorp/nomad/nomad/structs"
+)
 
 const (
 	AnnotationForcesCreate            = "forces create"
@@ -9,95 +14,167 @@ const (
 	AnnotationForcesDestructiveUpdate = "forces create/destroy update"
 )
 
-// Annotate takes the diff between the old and new version of a Job and will add
-// annotations to the diff to aide human understanding of the plan.
+// UpdateTypes denote the type of update to occur against the task group.
+const (
+	UpdateTypeIgnore            = "ignore"
+	UpdateTypeCreate            = "create"
+	UpdateTypeDestroy           = "destroy"
+	UpdateTypeMigrate           = "migrate"
+	UpdateTypeInplaceUpdate     = "in-place update"
+	UpdateTypeDestructiveUpdate = "create/destroy update"
+)
+
+// Annotate takes the diff between the old and new version of a Job, the
+// scheduler plan and will add annotations to the diff to aide human
+// understanding of the plan.
 //
 // Currently the things that are annotated are:
-// * Count up and count down changes
+// * Task group changes will be annotated with:
+//    * Count up and count down changes
+//    * Update counts (creates, destroys, migrates, etc)
 // * Task changes will be annotated with:
 //    * forces create/destroy update
 //    * forces in-place update
-func Annotate(diff *structs.JobDiff) {
+func Annotate(diff *structs.JobDiff, plan *structs.Plan) error {
 	// No annotation needed as the job was either just submitted or deleted.
 	if diff.Type != structs.DiffTypeEdited {
-		return
+		return nil
 	}
 
 	tgDiffs := diff.TaskGroups
-	if tgDiffs == nil {
-		return
+	if len(tgDiffs) == 0 {
+		return nil
 	}
 
-	for _, tgDiff := range tgDiffs.Edited {
-		annotateTaskGroup(tgDiff)
+	for _, tgDiff := range tgDiffs {
+		if err := annotateTaskGroup(tgDiff, plan); err != nil {
+			return err
+		}
 	}
+
+	return nil
 }
 
 // annotateTaskGroup takes a task group diff and annotates it.
-func annotateTaskGroup(diff *structs.TaskGroupDiff) {
+func annotateTaskGroup(diff *structs.TaskGroupDiff, plan *structs.Plan) error {
 	// Don't annotate unless the task group was edited. If it was a
 	// create/destroy it is not needed.
-	if diff == nil || diff.Type != structs.DiffTypeEdited {
-		return
+	if diff.Type != structs.DiffTypeEdited {
+		return nil
+	}
+
+	// Annotate the updates
+	if plan != nil && plan.Annotations != nil {
+		tg, ok := plan.Annotations.DesiredTGUpdates[diff.Name]
+		if ok {
+			if diff.Updates == nil {
+				diff.Updates = make(map[string]uint64, 6)
+			}
+
+			if tg.Ignore != 0 {
+				diff.Updates[UpdateTypeIgnore] = tg.Ignore
+			}
+			if tg.Place != 0 {
+				diff.Updates[UpdateTypeCreate] = tg.Place
+			}
+			if tg.Migrate != 0 {
+				diff.Updates[UpdateTypeMigrate] = tg.Migrate
+			}
+			if tg.Stop != 0 {
+				diff.Updates[UpdateTypeDestroy] = tg.Stop
+			}
+			if tg.InPlaceUpdate != 0 {
+				diff.Updates[UpdateTypeInplaceUpdate] = tg.InPlaceUpdate
+			}
+			if tg.DestructiveUpdate != 0 {
+				diff.Updates[UpdateTypeDestructiveUpdate] = tg.DestructiveUpdate
+			}
+		}
 	}
 
 	// Annotate the count
-	annotateCountChange(diff)
+	if err := annotateCountChange(diff); err != nil {
+		return err
+	}
 
 	// Annotate the tasks.
 	taskDiffs := diff.Tasks
-	if taskDiffs == nil {
-		return
+	if len(taskDiffs) == 0 {
+		return nil
 	}
 
-	for _, taskDiff := range taskDiffs.Edited {
+	for _, taskDiff := range taskDiffs {
 		annotateTask(taskDiff)
 	}
+
+	return nil
 }
 
 // annotateCountChange takes a task group diff and annotates the count
 // parameter.
-func annotateCountChange(diff *structs.TaskGroupDiff) {
+func annotateCountChange(diff *structs.TaskGroupDiff) error {
 	// Don't annotate unless the task was edited. If it was a create/destroy it
 	// is not needed.
-	if diff == nil || diff.Type != structs.DiffTypeEdited {
-		return
+	if diff.Type != structs.DiffTypeEdited {
+		return nil
 	}
 
-	countDiff, ok := diff.PrimitiveFields["Count"]
-	if !ok {
-		return
+	var countDiff *structs.FieldDiff
+	for _, diff := range diff.Fields {
+		if diff.Name == "Count" {
+			countDiff = diff
+			break
+		}
+	}
+
+	// Didn't find
+	if countDiff == nil {
+		fmt.Println("NO COUNT")
+		return nil
+	}
+	oldV, err := strconv.Atoi(countDiff.Old)
+	if err != nil {
+		return err
+	}
+
+	newV, err := strconv.Atoi(countDiff.New)
+	if err != nil {
+		return err
 	}
-	oldV := countDiff.OldValue.(int)
-	newV := countDiff.NewValue.(int)
 
 	if oldV < newV {
 		countDiff.Annotations = append(countDiff.Annotations, AnnotationForcesCreate)
 	} else if newV < oldV {
 		countDiff.Annotations = append(countDiff.Annotations, AnnotationForcesDestroy)
 	}
+
+	return nil
 }
 
 // annotateCountChange takes a task diff and annotates it.
 func annotateTask(diff *structs.TaskDiff) {
 	// Don't annotate unless the task was edited. If it was a create/destroy it
 	// is not needed.
-	if diff == nil || diff.Type != structs.DiffTypeEdited {
+	if diff.Type != structs.DiffTypeEdited {
 		return
 	}
 
 	// All changes to primitive fields result in a destructive update.
 	destructive := false
-	if len(diff.PrimitiveFields) != 0 {
+	if len(diff.Fields) != 0 {
 		destructive = true
 	}
 
-	if diff.Env != nil ||
-		diff.Meta != nil ||
-		diff.Artifacts != nil ||
-		diff.Resources != nil ||
-		diff.Config != nil {
-		destructive = true
+	// Changes that can be done in-place are log configs, services and
+	// constraints.
+	for _, oDiff := range diff.Objects {
+		switch oDiff.Name {
+		case "LogConfig", "Service", "Constraint":
+			continue
+		default:
+			destructive = true
+			break
+		}
 	}
 
 	if destructive {

scheduler/annotate_test.go

@@ -18,52 +18,40 @@ func TestAnnotateCountChange_NonEdited(t *testing.T) {
 
 func TestAnnotateCountChange(t *testing.T) {
 	up := &structs.FieldDiff{
-		DiffEntry: structs.DiffEntry{
-			Type: structs.DiffTypeEdited,
-		},
-		Name:     "Count",
-		OldValue: 1,
-		NewValue: 3,
+		Type: structs.DiffTypeEdited,
+		Name: "Count",
+		Old:  "1",
+		New:  "3",
 	}
 	down := &structs.FieldDiff{
-		DiffEntry: structs.DiffEntry{
-			Type: structs.DiffTypeEdited,
-		},
-		Name:     "Count",
-		OldValue: 3,
-		NewValue: 1,
+		Type: structs.DiffTypeEdited,
+		Name: "Count",
+		Old:  "3",
+		New:  "1",
 	}
 	tgUp := &structs.TaskGroupDiff{
-		PrimitiveStructDiff: structs.PrimitiveStructDiff{
-			DiffEntry: structs.DiffEntry{
-				Type: structs.DiffTypeEdited,
-			},
-			PrimitiveFields: map[string]*structs.FieldDiff{
-				"Count": up,
-			},
-		},
+		Type:   structs.DiffTypeEdited,
+		Fields: []*structs.FieldDiff{up},
 	}
 	tgDown := &structs.TaskGroupDiff{
-		PrimitiveStructDiff: structs.PrimitiveStructDiff{
-			DiffEntry: structs.DiffEntry{
-				Type: structs.DiffTypeEdited,
-			},
-			PrimitiveFields: map[string]*structs.FieldDiff{
-				"Count": down,
-			},
-		},
+		Type:   structs.DiffTypeEdited,
+		Fields: []*structs.FieldDiff{down},
 	}
 
 	// Test the up case
-	annotateCountChange(tgUp)
-	countDiff := tgUp.PrimitiveFields["Count"]
+	if err := annotateCountChange(tgUp); err != nil {
+		t.Fatalf("annotateCountChange(%#v) failed: %v", tgUp, err)
+	}
+	countDiff := tgUp.Fields[0]
 	if len(countDiff.Annotations) != 1 || countDiff.Annotations[0] != AnnotationForcesCreate {
 		t.Fatalf("incorrect annotation: %#v", tgUp)
 	}
 
 	// Test the down case
-	annotateCountChange(tgDown)
-	countDiff = tgDown.PrimitiveFields["Count"]
+	if err := annotateCountChange(tgDown); err != nil {
+		t.Fatalf("annotateCountChange(%#v) failed: %v", tgDown, err)
+	}
+	countDiff = tgDown.Fields[0]
 	if len(countDiff.Annotations) != 1 || countDiff.Annotations[0] != AnnotationForcesDestroy {
 		t.Fatalf("incorrect annotation: %#v", tgDown)
 	}
@@ -81,93 +69,126 @@ func TestAnnotateTask_NonEdited(t *testing.T) {
 func TestAnnotateTask_Destructive(t *testing.T) {
 	cases := []*structs.TaskDiff{
 		{
-			PrimitiveStructDiff: structs.PrimitiveStructDiff{
-				DiffEntry: structs.DiffEntry{
+			Type: structs.DiffTypeEdited,
+			Fields: []*structs.FieldDiff{
+				{
 					Type: structs.DiffTypeEdited,
-				},
-				PrimitiveFields: map[string]*structs.FieldDiff{
-					"Driver": &structs.FieldDiff{
-						Name:     "Driver",
-						OldValue: "docker",
-						NewValue: "exec",
-					},
+					Name: "Driver",
+					Old:  "docker",
+					New:  "exec",
 				},
 			},
 		},
 		{
-			PrimitiveStructDiff: structs.PrimitiveStructDiff{
-				DiffEntry: structs.DiffEntry{
+			Type: structs.DiffTypeEdited,
+			Fields: []*structs.FieldDiff{
+				{
 					Type: structs.DiffTypeEdited,
-				},
-				PrimitiveFields: map[string]*structs.FieldDiff{
-					"User": &structs.FieldDiff{
-						Name:     "User",
-						OldValue: "",
-						NewValue: "specific",
-					},
+					Name: "User",
+					Old:  "alice",
+					New:  "bob",
 				},
 			},
 		},
 		{
-			Env: &structs.StringMapDiff{
-				DiffEntry: structs.DiffEntry{
+			Type: structs.DiffTypeEdited,
+			Fields: []*structs.FieldDiff{
+				{
 					Type: structs.DiffTypeAdded,
-				},
-				Added: map[string]string{
-					"foo": "bar",
+					Name: "Env[foo]",
+					Old:  "foo",
+					New:  "bar",
 				},
 			},
 		},
 		{
-			Meta: &structs.StringMapDiff{
-				DiffEntry: structs.DiffEntry{
-					Type: structs.DiffTypeEdited,
-				},
-				Edited: map[string]structs.StringValueDelta{
-					"foo": {
-						Old: "a",
-						New: "b",
-					},
-				},
-			},
-		},
-		{
-			Artifacts: &structs.TaskArtifactsDiff{
-				DiffEntry: structs.DiffEntry{
+			Type: structs.DiffTypeEdited,
+			Fields: []*structs.FieldDiff{
+				{
 					Type: structs.DiffTypeAdded,
-				},
-				Added: []*structs.TaskArtifact{
-					{
-						GetterSource: "foo",
-					},
+					Name: "Meta[foo]",
+					Old:  "foo",
+					New:  "bar",
 				},
 			},
 		},
 		{
-			Resources: &structs.ResourcesDiff{
-				PrimitiveStructDiff: structs.PrimitiveStructDiff{
-					DiffEntry: structs.DiffEntry{
-						Type: structs.DiffTypeEdited,
-					},
-					PrimitiveFields: map[string]*structs.FieldDiff{
-						"CPU": &structs.FieldDiff{
-							Name:     "CPU",
-							OldValue: 500,
-							NewValue: 1000,
+			Type: structs.DiffTypeEdited,
+			Objects: []*structs.ObjectDiff{
+				{
+					Type: structs.DiffTypeAdded,
+					Name: "Artifact",
+					Fields: []*structs.FieldDiff{
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "GetterOptions[bam]",
+							Old:  "",
+							New:  "baz",
+						},
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "GetterSource",
+							Old:  "",
+							New:  "bam",
+						},
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "RelativeDest",
+							Old:  "",
+							New:  "bam",
 						},
 					},
 				},
 			},
 		},
 		{
-			Config: &structs.StringMapDiff{
-				DiffEntry: structs.DiffEntry{
+			Type: structs.DiffTypeEdited,
+			Objects: []*structs.ObjectDiff{
+				{
 					Type: structs.DiffTypeEdited,
+					Name: "Resources",
+					Fields: []*structs.FieldDiff{
+						{
+							Type: structs.DiffTypeEdited,
+							Name: "CPU",
+							Old:  "100",
+							New:  "200",
+						},
+						{
+							Type: structs.DiffTypeEdited,
+							Name: "DiskMB",
+							Old:  "100",
+							New:  "200",
+						},
+						{
+							Type: structs.DiffTypeEdited,
+							Name: "IOPS",
+							Old:  "100",
+							New:  "200",
+						},
+						{
+							Type: structs.DiffTypeEdited,
+							Name: "MemoryMB",
+							Old:  "100",
+							New:  "200",
+						},
+					},
 				},
-				Edited: map[string]structs.StringValueDelta{
-					"command": {
-						Old: "/bin/date",
-						New: "/bin/bash",
+			},
+		},
+		{
+			Type: structs.DiffTypeEdited,
+			Objects: []*structs.ObjectDiff{
+				{
+					Type: structs.DiffTypeEdited,
+					Name: "Config",
+					Fields: []*structs.FieldDiff{
+						{
+							Type: structs.DiffTypeEdited,
+							Name: "bam[1]",
+							Old:  "b",
+							New:  "c",
+						},
 					},
 				},
 			},
@@ -186,44 +207,70 @@ func TestAnnotateTask_Destructive(t *testing.T) {
 func TestAnnotateTask_Inplace(t *testing.T) {
 	cases := []*structs.TaskDiff{
 		{
-			Constraints: []*structs.PrimitiveStructDiff{
+			Type: structs.DiffTypeEdited,
+			Objects: []*structs.ObjectDiff{
 				{
-					DiffEntry: structs.DiffEntry{
-						Type: structs.DiffTypeEdited,
-					},
-					PrimitiveFields: map[string]*structs.FieldDiff{
-						"RTarget": &structs.FieldDiff{
-							Name:     "RTarget",
-							OldValue: "linux",
-							NewValue: "windows",
+					Type: structs.DiffTypeAdded,
+					Name: "Constraint",
+					Fields: []*structs.FieldDiff{
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "LTarget",
+							Old:  "",
+							New:  "baz",
+						},
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "Operand",
+							Old:  "",
+							New:  "baz",
+						},
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "RTarget",
+							Old:  "",
+							New:  "baz",
 						},
 					},
 				},
 			},
 		},
 		{
-			LogConfig: &structs.PrimitiveStructDiff{
-				DiffEntry: structs.DiffEntry{
-					Type: structs.DiffTypeEdited,
-				},
-				PrimitiveFields: map[string]*structs.FieldDiff{
-					"MaxFileSizeMB": &structs.FieldDiff{
-						Name:     "MaxFileSizeMB",
-						OldValue: 100,
-						NewValue: 128,
+			Type: structs.DiffTypeEdited,
+			Objects: []*structs.ObjectDiff{
+				{
+					Type: structs.DiffTypeAdded,
+					Name: "LogConfig",
+					Fields: []*structs.FieldDiff{
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "MaxFileSizeMB",
+							Old:  "",
+							New:  "10",
+						},
+						{
+							Type: structs.DiffTypeAdded,
+							Name: "MaxFiles",
+							Old:  "",
+							New:  "1",
+						},
 					},
 				},
 			},
 		},
 		{
-			Services: &structs.ServicesDiff{
-				DiffEntry: structs.DiffEntry{
-					Type: structs.DiffTypeAdded,
-				},
-				Added: []*structs.Service{
-					{
-						Name:      "foo",
-						PortLabel: "rpc",
+			Type: structs.DiffTypeEdited,
+			Objects: []*structs.ObjectDiff{
+				{
+					Type: structs.DiffTypeEdited,
+					Name: "Service",
+					Fields: []*structs.FieldDiff{
+						{
+							Type: structs.DiffTypeEdited,
+							Name: "PortLabel",
+							Old:  "baz",
+							New:  "baz2",
+						},
 					},
 				},
 			},

scheduler/util.go

@@ -480,7 +480,7 @@ func desiredUpdates(diff *diffResult, inplaceUpdates,
 	}
 
 	for _, tuple := range diff.stop {
-		name := tuple.Alloc.TaskGroup
+		name := tuple.TaskGroup.Name
 		des, ok := desiredTgs[name]
 		if !ok {
 			des = &structs.DesiredUpdates{}

scheduler/util_test.go

@@ -765,6 +765,7 @@ func TestProgressMade(t *testing.T) {
 func TestDesiredUpdates(t *testing.T) {
 	tg1 := &structs.TaskGroup{Name: "foo"}
 	tg2 := &structs.TaskGroup{Name: "bar"}
+	a2 := &structs.Allocation{TaskGroup: "foo"}
 
 	place := []allocTuple{
 		allocTuple{TaskGroup: tg1},
@@ -773,8 +774,8 @@ func TestDesiredUpdates(t *testing.T) {
 		allocTuple{TaskGroup: tg2},
 	}
 	stop := []allocTuple{
-		allocTuple{TaskGroup: tg2},
-		allocTuple{TaskGroup: tg2},
+		allocTuple{TaskGroup: tg2, Alloc: a2},
+		allocTuple{TaskGroup: tg2, Alloc: a2},
 	}
 	ignore := []allocTuple{
 		allocTuple{TaskGroup: tg1},