diff --git a/.changelog/10965.txt b/.changelog/10965.txt new file mode 100644 index 000000000000..a60bf367d97d --- /dev/null +++ b/.changelog/10965.txt @@ -0,0 +1,3 @@ +```release-note:bug +core: Fixed an issue that created incorrect plan output for jobs with services with the same name. +``` diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 24d1f242e14b..845c37c5a520 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -643,29 +643,57 @@ func serviceDiff(old, new *Service, contextual bool) *ObjectDiff { // serviceDiffs diffs a set of services. If contextual diff is enabled, unchanged // fields within objects nested in the tasks will be returned. func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff { - oldMap := make(map[string]*Service, len(old)) - newMap := make(map[string]*Service, len(new)) - for _, o := range old { - oldMap[o.Name] = o + // Handle trivial case. + if len(old) == 1 && len(new) == 1 { + if diff := serviceDiff(old[0], new[0], contextual); diff != nil { + return []*ObjectDiff{diff} + } + return nil } - for _, n := range new { - newMap[n.Name] = n + + // For each service we will try to find a corresponding match in the other + // service list. + // The following lists store the index of the matching service for each + // position of the inputs. + oldMatches := make([]int, len(old)) + newMatches := make([]int, len(new)) + + // Initialize all services as unmatched. + for i := range oldMatches { + oldMatches[i] = -1 + } + for i := range newMatches { + newMatches[i] = -1 } + // Find a match in the new services list for each old service and compute + // their diffs. var diffs []*ObjectDiff - for name, oldService := range oldMap { - // Diff the same, deleted and edited - if diff := serviceDiff(oldService, newMap[name], contextual); diff != nil { + for oldIndex, oldService := range old { + newIndex := findServiceMatch(oldService, oldIndex, new, newMatches) + + // Old services that don't have a match were deleted. + if newIndex < 0 { + diff := serviceDiff(oldService, nil, contextual) + diffs = append(diffs, diff) + continue + } + + // If A matches B then B matches A. + oldMatches[oldIndex] = newIndex + newMatches[newIndex] = oldIndex + + newService := new[newIndex] + if diff := serviceDiff(oldService, newService, contextual); diff != nil { diffs = append(diffs, diff) } } - for name, newService := range newMap { - // Diff the added - if old, ok := oldMap[name]; !ok { - if diff := serviceDiff(old, newService, contextual); diff != nil { - diffs = append(diffs, diff) - } + // New services without match were added. + for i, m := range newMatches { + if m == -1 { + diff := serviceDiff(nil, new[i], contextual) + diffs = append(diffs, diff) } } @@ -673,6 +701,74 @@ func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff { return diffs } +// findServiceMatch returns the index of the service in the input services list +// that matches the provided input service. +func findServiceMatch(service *Service, serviceIndex int, services []*Service, matches []int) int { + // minScoreThreshold can be adjusted to generate more (lower value) or + // fewer (higher value) matches. + // More matches result in more Edited diffs, while fewer matches generate + // more Add/Delete diff pairs. + minScoreThreshold := 2 + + highestScore := 0 + indexMatch := -1 + + for i, s := range services { + // Skip service if it's already matched. + if matches[i] >= 0 { + continue + } + + // Finding a perfect match by just looking at the before and after + // list of services is impossible since they don't have a stable + // identifier that can be used to uniquely identify them. + // + // Users also have an implicit temporal intuition of which services + // match each other when editing their jobspec file. If they move the + // 3rd service to the top, they don't expect their job to change. + // + // This intuition could be made explicit by requiring a user-defined + // unique identifier, but this would cause additional work and the + // new field would not be intuitive for users to understand how to use + // it. + // + // Using a hash value of the service content will cause any changes to + // create a delete/add diff pair. + // + // There are three main candidates for a service ID: + // - name, but they are not unique and can be modified. + // - label port, but they have the same problems as name. + // - service position within the overall list of services, but if the + // service block is moved, it will impact all services that come + // after it. + // + // None of these values are enough on their own, but they are also too + // strong when considered all together. + // + // So we try to score services by their main candidates with a preference + // towards name + label over service position. + score := 0 + if i == serviceIndex { + score += 1 + } + + if service.PortLabel == s.PortLabel { + score += 2 + } + + if service.Name == s.Name { + score += 3 + } + + if score > minScoreThreshold && score > highestScore { + highestScore = score + indexMatch = i + } + } + + return indexMatch +} + // serviceCheckDiff returns the diff of two service check objects. If contextual // diff is enabled, all fields will be returned, even if no diff occurred. func serviceCheckDiff(old, new *ServiceCheck, contextual bool) *ObjectDiff { diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 76221d5a208c..6410ce4af1dc 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7187,3 +7187,411 @@ func TestTaskDiff(t *testing.T) { }) } } + +func TestServicesDiff(t *testing.T) { + cases := []struct { + Name string + Old, New []*Service + Expected []*ObjectDiff + Contextual bool + }{ + { + Name: "No changes - empty", + Contextual: true, + Old: []*Service{}, + New: []*Service{}, + Expected: nil, + }, + { + Name: "No changes", + Contextual: true, + Old: []*Service{ + { + Name: "webapp", + PortLabel: "http", + }, + }, + New: []*Service{ + { + Name: "webapp", + PortLabel: "http", + }, + }, + Expected: nil, + }, + { + Name: "Detect changes", + Contextual: true, + Old: []*Service{ + { + Name: "webapp", + PortLabel: "http", + AddressMode: "host", + EnableTagOverride: true, + Tags: []string{"prod"}, + CanaryTags: []string{"canary"}, + }, + }, + New: []*Service{ + { + Name: "webapp-2", + PortLabel: "https", + AddressMode: "alloc", + EnableTagOverride: false, + Tags: []string{"prod", "dev"}, + CanaryTags: []string{"qa"}, + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeEdited, + Name: "AddressMode", + Old: "host", + New: "alloc", + }, + { + Type: DiffTypeEdited, + Name: "EnableTagOverride", + Old: "true", + New: "false", + }, + { + Type: DiffTypeEdited, + Name: "Name", + Old: "webapp", + New: "webapp-2", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeEdited, + Name: "PortLabel", + Old: "http", + New: "https", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "CanaryTags", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "CanaryTags", + New: "qa", + }, + { + Type: DiffTypeDeleted, + Name: "CanaryTags", + Old: "canary", + }, + }, + }, + { + Type: DiffTypeAdded, + Name: "Tags", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "Tags", + New: "dev", + }, + { + Type: DiffTypeNone, + Name: "Tags", + Old: "prod", + New: "prod", + }, + }, + }, + }, + }, + }, + }, + { + Name: "Service added", + Contextual: true, + Old: []*Service{}, + New: []*Service{ + { + Name: "webapp", + PortLabel: "http", + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeNone, + Name: "AddressMode", + }, + { + Type: DiffTypeAdded, + Name: "EnableTagOverride", + New: "false", + }, + { + Type: DiffTypeAdded, + Name: "Name", + New: "webapp", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeAdded, + Name: "PortLabel", + New: "http", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + }, + }, + }, + { + Name: "Service added with same name", + Contextual: true, + Old: []*Service{ + { + Name: "webapp", + PortLabel: "http", + }, + }, + New: []*Service{ + { + Name: "webapp", + PortLabel: "https", + }, + { + Name: "webapp", + PortLabel: "http", + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeNone, + Name: "AddressMode", + }, + { + Type: DiffTypeAdded, + Name: "EnableTagOverride", + New: "false", + }, + { + Type: DiffTypeAdded, + Name: "Name", + New: "webapp", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeAdded, + Name: "PortLabel", + New: "https", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + }, + }, + }, + { + Name: "Modify port label of service with same name", + Contextual: true, + Old: []*Service{ + { + Name: "webapp", + PortLabel: "http", + }, + { + Name: "webapp", + PortLabel: "https", + }, + }, + New: []*Service{ + { + Name: "webapp", + PortLabel: "https-redirect", + }, + { + Name: "webapp", + PortLabel: "https", + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeNone, + Name: "AddressMode", + }, + { + Type: DiffTypeNone, + Name: "EnableTagOverride", + Old: "false", + New: "false", + }, + { + Type: DiffTypeNone, + Name: "Name", + Old: "webapp", + New: "webapp", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeEdited, + Name: "PortLabel", + Old: "http", + New: "https-redirect", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + }, + }, + }, + { + Name: "Modify similar services", + Contextual: true, + Old: []*Service{ + { + Name: "webapp", + PortLabel: "http", + Tags: []string{"prod"}, + }, + { + Name: "webapp", + PortLabel: "http", + Tags: []string{"dev"}, + }, + }, + New: []*Service{ + { + Name: "webapp", + PortLabel: "http", + Tags: []string{"prod", "qa"}, + }, + { + Name: "webapp", + PortLabel: "http", + Tags: []string{"dev"}, + }, + }, + Expected: []*ObjectDiff{ + { + Type: DiffTypeEdited, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeNone, + Name: "AddressMode", + }, + { + Type: DiffTypeNone, + Name: "EnableTagOverride", + Old: "false", + New: "false", + }, + { + Type: DiffTypeNone, + Name: "Name", + Old: "webapp", + New: "webapp", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeNone, + Name: "PortLabel", + Old: "http", + New: "http", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Tags", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "Tags", + New: "qa", + }, + { + Type: DiffTypeNone, + Name: "Tags", + Old: "prod", + New: "prod", + }, + }, + }, + }, + }, + }, + }, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + actual := serviceDiffs(c.Old, c.New, c.Contextual) + require.Equal(t, c.Expected, actual) + }) + } +}