diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index b00ccffe5..1e567a150 100644 --- a/nomad/job_endpoint.go +++ b/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 } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 6cd8a5be2..c70af0f76 100644 --- a/nomad/structs/diff.go +++ b/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 { diff --git a/scheduler/annotate.go b/scheduler/annotate.go index 00209f921..559e640b7 100644 --- a/scheduler/annotate.go +++ b/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 { diff --git a/scheduler/annotate_test.go b/scheduler/annotate_test.go index b22174267..5f90ef23f 100644 --- a/scheduler/annotate_test.go +++ b/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", + }, }, }, }, diff --git a/scheduler/util.go b/scheduler/util.go index 15c38c479..120440918 100644 --- a/scheduler/util.go +++ b/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{} diff --git a/scheduler/util_test.go b/scheduler/util_test.go index 4d63ff195..338f92d3c 100644 --- a/scheduler/util_test.go +++ b/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},