Fixed plan diffing to handle non-unique service names. (#10965)

.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.
+```

nomad/structs/diff.go

@@ -643,36 +643,132 @@ 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)
 		}
 	}
 
 	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 {
+	// 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 {