Fixed plan diffing to handle non-unique service names. (#10965)
This commit is contained in:
Florian Apolloner
GitHub
parent
c0023c6c85
commit
511cae92b4
|
|
@ -0,0 +1,3 @@
|
|||
```release-note:bug
|
||||
core: Fixed an issue that created incorrect plan output for jobs with services with the same name.
|
||||
```
|
||||
|
|
@ -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
|
||||
}
|
||||
for _, n := range new {
|
||||
newMap[n.Name] = n
|
||||
// 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 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 {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue