From b18e8c428e6f270f366ae97e28460512c7209feb Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Thu, 29 Jul 2021 10:46:11 +0200 Subject: [PATCH 1/6] Fixed plan diffing to handle non-unique service names. --- .changelog/10965.txt | 3 ++ nomad/structs/diff.go | 60 ++++++++++++++++++++++++++++----- nomad/structs/diff_test.go | 68 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 9 deletions(-) create mode 100644 .changelog/10965.txt diff --git a/.changelog/10965.txt b/.changelog/10965.txt new file mode 100644 index 000000000000..7778c5e07795 --- /dev/null +++ b/.changelog/10965.txt @@ -0,0 +1,3 @@ +```release-note:bug +plan: Handle multiple services with the same name. +``` diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 24d1f242e14b..7912ca029e60 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -640,35 +640,77 @@ func serviceDiff(old, new *Service, contextual bool) *ObjectDiff { return diff } -// 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 { +// serviceDiffsInternal diffs a set of services with the same name. +func serviceDiffsInternal(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 + oldMap[o.PortLabel] = o } for _, n := range new { - newMap[n.Name] = n + newMap[n.PortLabel] = n } var diffs []*ObjectDiff - for name, oldService := range oldMap { + for portLabel, oldService := range oldMap { // Diff the same, deleted and edited - if diff := serviceDiff(oldService, newMap[name], contextual); diff != nil { + if diff := serviceDiff(oldService, newMap[portLabel], contextual); diff != nil { diffs = append(diffs, diff) } } - for name, newService := range newMap { + for portLabel, newService := range newMap { // Diff the added - if old, ok := oldMap[name]; !ok { + if old, ok := oldMap[portLabel]; !ok { if diff := serviceDiff(old, newService, contextual); diff != nil { diffs = append(diffs, diff) } } } + return diffs +} + +// 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] = append(oldMap[o.Name], o) + } + for _, n := range new { + newMap[n.Name] = append(newMap[n.Name], n) + } + + var diffs []*ObjectDiff + for name, oldServices := range oldMap { + // services were deleted + if newServices, ok := newMap[name]; !ok { + for _, oldService := range oldServices { + diffs = append(diffs, serviceDiff(oldService, nil, contextual)) + } + } else { // services where changed + // Handle the case of a "simple" change + if len(newServices) == 1 && len(oldServices) == 1 { + if diff := serviceDiff(oldServices[0], newServices[0], contextual); diff != nil { + diffs = append(diffs, diff) + } + } else { // More complex changes + diffs = append(diffs, serviceDiffsInternal(oldServices, newServices, contextual)...) + } + } + } + + for name, newServices := range newMap { + // services where added + if _, ok := oldMap[name]; !ok { + for _, newService := range newServices { + diffs = append(diffs, serviceDiff(nil, newService, contextual)) + } + } + } + sort.Sort(ObjectDiffs(diffs)) return diffs } diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 6eabcdc8b004..83132d8bc1ea 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -3885,6 +3885,74 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, + + { + TestCase: "TaskGroup service added with same name", + Contextual: true, + Old: &TaskGroup{ + Services: []*Service{ + { + Name: "foo", + PortLabel: "label1", + }, + }, + }, + + New: &TaskGroup{ + Services: []*Service{ + { + Name: "foo", + PortLabel: "label1", + }, + { + Name: "foo", + PortLabel: "label2", + }, + }, + }, + Expected: &TaskGroupDiff{ + Type: DiffTypeEdited, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Service", + Fields: []*FieldDiff{ + { + Type: DiffTypeNone, + Name: "AddressMode", + }, + { + Type: DiffTypeAdded, + Name: "EnableTagOverride", + New: "false", + }, + { + Type: DiffTypeAdded, + Name: "Name", + New: "foo", + }, + { + Type: DiffTypeNone, + Name: "Namespace", + }, + { + Type: DiffTypeNone, + Name: "OnUpdate", + }, + { + Type: DiffTypeAdded, + Name: "PortLabel", + New: "label2", + }, + { + Type: DiffTypeNone, + Name: "TaskName", + }, + }, + }, + }, + }, + }, } for i, c := range cases { From d68c5279af622a214979e2334b5c73925e6813d0 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 7 Sep 2021 17:24:45 -0400 Subject: [PATCH 2/6] consider service name, port label and position when computing diff --- nomad/structs/diff.go | 153 +++++++----- nomad/structs/diff_test.go | 476 +++++++++++++++++++++++++++++++------ 2 files changed, 508 insertions(+), 121 deletions(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 7912ca029e60..72ccadb0bba7 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -640,79 +640,126 @@ func serviceDiff(old, new *Service, contextual bool) *ObjectDiff { return diff } -// serviceDiffsInternal diffs a set of services with the same name. -func serviceDiffsInternal(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.PortLabel] = o +// 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 { + // 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.PortLabel] = 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 portLabel, oldService := range oldMap { - // Diff the same, deleted and edited - if diff := serviceDiff(oldService, newMap[portLabel], contextual); diff != nil { + for oldIndex, oldService := range old { + newIndex := findServiceMatch(oldService, oldIndex, new, newMatches) + if newIndex < 0 { + 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 portLabel, newService := range newMap { - // Diff the added - if old, ok := oldMap[portLabel]; !ok { - if diff := serviceDiff(old, newService, contextual); diff != nil { - diffs = append(diffs, diff) - } + // Old services without match were deleted. + for i, m := range oldMatches { + if m == -1 { + diff := serviceDiff(old[i], nil, contextual) + 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) } } return diffs } -// 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] = append(oldMap[o.Name], o) - } - for _, n := range new { - newMap[n.Name] = append(newMap[n.Name], n) - } +// 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 { + for i, s := range services { + // Skip service if it's already matched. + if matches[i] >= 0 { + continue + } - var diffs []*ObjectDiff - for name, oldServices := range oldMap { - // services were deleted - if newServices, ok := newMap[name]; !ok { - for _, oldService := range oldServices { - diffs = append(diffs, serviceDiff(oldService, nil, contextual)) - } - } else { // services where changed - // Handle the case of a "simple" change - if len(newServices) == 1 && len(oldServices) == 1 { - if diff := serviceDiff(oldServices[0], newServices[0], contextual); diff != nil { - diffs = append(diffs, diff) - } - } else { // More complex changes - diffs = append(diffs, serviceDiffsInternal(oldServices, newServices, contextual)...) - } + // 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 consider two services to match if at least 2 of these 3 values + // are equal. + score := 0 + if i == serviceIndex { + score += 1 } - } - for name, newServices := range newMap { - // services where added - if _, ok := oldMap[name]; !ok { - for _, newService := range newServices { - diffs = append(diffs, serviceDiff(nil, newService, contextual)) - } + if service.Name == s.Name { + score += 1 + } + + if service.PortLabel == s.PortLabel { + score += 1 + } + + if score >= 2 { + return i } } - sort.Sort(ObjectDiffs(diffs)) - return diffs + return -1 } // serviceCheckDiff returns the diff of two service check objects. If contextual diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 83132d8bc1ea..c1c04af760ca 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -3885,74 +3885,6 @@ func TestTaskGroupDiff(t *testing.T) { }, }, }, - - { - TestCase: "TaskGroup service added with same name", - Contextual: true, - Old: &TaskGroup{ - Services: []*Service{ - { - Name: "foo", - PortLabel: "label1", - }, - }, - }, - - New: &TaskGroup{ - Services: []*Service{ - { - Name: "foo", - PortLabel: "label1", - }, - { - Name: "foo", - PortLabel: "label2", - }, - }, - }, - Expected: &TaskGroupDiff{ - Type: DiffTypeEdited, - Objects: []*ObjectDiff{ - { - Type: DiffTypeAdded, - Name: "Service", - Fields: []*FieldDiff{ - { - Type: DiffTypeNone, - Name: "AddressMode", - }, - { - Type: DiffTypeAdded, - Name: "EnableTagOverride", - New: "false", - }, - { - Type: DiffTypeAdded, - Name: "Name", - New: "foo", - }, - { - Type: DiffTypeNone, - Name: "Namespace", - }, - { - Type: DiffTypeNone, - Name: "OnUpdate", - }, - { - Type: DiffTypeAdded, - Name: "PortLabel", - New: "label2", - }, - { - Type: DiffTypeNone, - Name: "TaskName", - }, - }, - }, - }, - }, - }, } for i, c := range cases { @@ -7241,3 +7173,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: "http", + }, + { + Name: "webapp", + PortLabel: "https", + }, + }, + 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) + }) + } +} From 7e336567dbf810f96ef4bbb13ca4b0c796a24f2e Mon Sep 17 00:00:00 2001 From: Florian Apolloner Date: Wed, 8 Sep 2021 07:59:56 +0200 Subject: [PATCH 3/6] Scoring fixes. --- nomad/structs/diff.go | 34 +++++++++++++++++++--------------- nomad/structs/diff_test.go | 4 ++-- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 72ccadb0bba7..ac1d9b5388a0 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -685,18 +685,18 @@ func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff { } } - // Old services without match were deleted. - for i, m := range oldMatches { + // New services without match were added. + for i, m := range newMatches { if m == -1 { - diff := serviceDiff(old[i], nil, contextual) + diff := serviceDiff(nil, new[i], contextual) diffs = append(diffs, diff) } } - // New services without match were added. - for i, m := range newMatches { + // Old services without match were deleted. + for i, m := range oldMatches { if m == -1 { - diff := serviceDiff(nil, new[i], contextual) + diff := serviceDiff(old[i], nil, contextual) diffs = append(diffs, diff) } } @@ -707,6 +707,9 @@ func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff { // 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 { + oldScore := -1 + newIndex := -1 + for i, s := range services { // Skip service if it's already matched. if matches[i] >= 0 { @@ -739,27 +742,28 @@ func findServiceMatch(service *Service, serviceIndex int, services []*Service, m // None of these values are enough on their own, but they are also too // strong when considered all together. // - // So we consider two services to match if at least 2 of these 3 values - // are equal. + // 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.Name == s.Name { - score += 1 + if service.PortLabel == s.PortLabel { + score += 2 } - if service.PortLabel == s.PortLabel { - score += 1 + if service.Name == s.Name { + score += 3 } - if score >= 2 { - return i + if score > 2 && score > oldScore { + oldScore = score + newIndex = i } } - return -1 + return newIndex } // serviceCheckDiff returns the diff of two service check objects. If contextual diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index c1c04af760ca..ad7d4ca6cbf9 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7370,11 +7370,11 @@ func TestServicesDiff(t *testing.T) { New: []*Service{ { Name: "webapp", - PortLabel: "http", + PortLabel: "https", }, { Name: "webapp", - PortLabel: "https", + PortLabel: "http", }, }, Expected: []*ObjectDiff{ From 87e43ec6c255f380b3d393469b2cac111eaad929 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 8 Sep 2021 14:50:37 -0400 Subject: [PATCH 4/6] minor code improvements around service diff calculation --- nomad/structs/diff.go | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index ac1d9b5388a0..cc669261fd49 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -671,7 +671,11 @@ func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff { var diffs []*ObjectDiff 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 } @@ -693,22 +697,21 @@ func serviceDiffs(old, new []*Service, contextual bool) []*ObjectDiff { } } - // Old services without match were deleted. - for i, m := range oldMatches { - if m == -1 { - diff := serviceDiff(old[i], nil, contextual) - diffs = append(diffs, diff) - } - } - + sort.Sort(ObjectDiffs(diffs)) 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 { - oldScore := -1 - newIndex := -1 + // 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 := 3 + + highestScore := 0 + indexMatch := -1 for i, s := range services { // Skip service if it's already matched. @@ -757,13 +760,13 @@ func findServiceMatch(service *Service, serviceIndex int, services []*Service, m score += 3 } - if score > 2 && score > oldScore { - oldScore = score - newIndex = i + if score > minScoreThreshold && score > highestScore { + highestScore = score + indexMatch = i } } - return newIndex + return indexMatch } // serviceCheckDiff returns the diff of two service check objects. If contextual From c0d2cbd3af229865be441381b9fbd2bd76f07254 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Wed, 8 Sep 2021 17:26:49 -0400 Subject: [PATCH 5/6] reduce service diff score threshold --- nomad/structs/diff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index cc669261fd49..845c37c5a520 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -708,7 +708,7 @@ func findServiceMatch(service *Service, serviceIndex int, services []*Service, m // fewer (higher value) matches. // More matches result in more Edited diffs, while fewer matches generate // more Add/Delete diff pairs. - minScoreThreshold := 3 + minScoreThreshold := 2 highestScore := 0 indexMatch := -1 From 784a84be8072638fb2a8e8b810caa91d2c15c8f8 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Fri, 8 Oct 2021 17:30:13 -0400 Subject: [PATCH 6/6] changelog: update entry for #10965 --- .changelog/10965.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/10965.txt b/.changelog/10965.txt index 7778c5e07795..a60bf367d97d 100644 --- a/.changelog/10965.txt +++ b/.changelog/10965.txt @@ -1,3 +1,3 @@ ```release-note:bug -plan: Handle multiple services with the same name. +core: Fixed an issue that created incorrect plan output for jobs with services with the same name. ```