Fixed plan diffing to handle non-unique service names.

[apollo13]

This is a draft for #10964 -- What it does not handle now anymore is a change in port label. Still brainstorming on how to handle this properly :)

Edit: Fixes #10964 -- trying to get the issue ui notice it as linked PR

[apollo13]

@shoenig If you have the capacity I'd love to get that into 1.1.3

[shoenig]

I think really the correct thing to do here would be to leverage .Hash as they key for Service. It'll take a little plumbing because Hash takes in paramters for allocID, taskName, and canary. You can see from other calls of Hash what we feed in for group level services. ... Or actually I think we can get away with passing in zero values for allocID and taskName, because in diff.go we aren't concerned with services outside of the scope of the group or task we are diffing.

LMK if that doesn't quite make sense 😅

[apollo13]

I am not sure hash as key makes sense (at least it would make diffing endlessly harder). Imagine you are changing a check in a service. This will obviously change the hash. How do you know which services to compare now to get a minimal diff? Or differently put: changing the key to the hash and not doing anything else would mean that any change in a service would result in diff showing it as deletion and addition. My approach tries to keep the diff minimal in the sense that if the name and portlabel stay the same it is most likely a change and in the service.

…

On Fri, Jul 30, 2021, at 16:50, Seth Hoenig wrote: I think really the correct thing to do here would be to leverage `.Hash` as they key for `Service`. It'll take a little plumbing because `Hash` takes in paramters for `allocID`, `taskName`, and `canary`. You can see from other calls of `Hash` what we feed in for group level services. ... Or actually I think we can get away with passing in zero values for `allocID` and `taskName`, because in `diff.go` we aren't concerned with services outside of the scope of the group or task we are diffing. LMK if that doesn't quite make sense 😅 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10965 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C43YRS65B5P7Q6D4DTT2K343ANCNFSM5BF743VQ>.

[apollo13]

@shoenig Not trying to be annoying, but any chance you could give this another review?

[lgfa29]

Hi @apollo13, sorry for the delay on getting this review.

This was a nice catch, but the more I thought about the more tricky I realized it was 😅

The heart of the problem is that services don't have a way to be uniquely identified. Every field can be changed, so it's impossible to keep track of which service is which when computing a diff.

Users usually have an intuition of this because they have this implicit notion of what is being modified as they edit the file, but when looking at just a before and after snapshot it's impossible to tell what modification were actually made. There is a similar problem in the frontend world, where the library needs to keep track of objects in a page. Unless the developer tells the library which object is which, it's impossible to figure out exactly. Here is a nice visual explanation: https://twitter.com/dan_abramov/status/1415279090446204929.

Your fix was great, but I wondered if it would be possible to get even better diffs if we considered even another facet of services: their relative position within the services list.

So I came up with #11148. I played around with it a bit, and also wrote some extra tests (I also moved your test to a new function because wow that TestTaskGroupDiff is long 😄) and it seems to work. I also think the final code is simpler and easier to understand, but I would love to hear your thoughts.

If that code works, we can cherry-pick it here and have your PR (finally) merged 🙂

[apollo13]

Hi @apollo13, sorry for the delay on getting this review.

Hi @lgfa29 -- no worries, thanks for getting to it.

This was a nice catch, but the more I thought about the more tricky I realized it was 😅

Oh yes, now imagine me when I ran into this rabbit hole :)

The heart of the problem is that services don't have a way to be uniquely identified. Every field can be changed, so it's impossible to keep track of which service is which when computing a diff.

That is a perfect summary, I couldn't have put it any better. Given that I think we should a) find a simple solution and b) focus on common cases. I think users can accept a sub optimal diff output in edge cases as long as it is at least technically correct (which the current one is not -- and I wish I knew that when I started using nomad since I always though I could just have one service with a name).

Your fix was great, but I wondered if it would be possible to get even better diffs if we considered even another facet of services: their relative position within the services list.

Talking about user intuition that is certainly a good idea.

If that code works, we can cherry-pick it here and have your PR (finally) merged 🙂

Well you do have the tests to show that your approach works. Where it falls down though is if I change the following:

diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
index c1c04af76..3ff0af206 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{

(That is when adding a new service with the same name before the existing one -- may be because the user sorts port labels alphabetically or whatever).

Thinking about user intuition I am actually not sure what the best approach would be. Yours is probably as reasonable as mine and mine uses "service + port label" as rather strong identifier that the service did not change (At least that is what I was trying to put in code). I'd argue that from a user perspective one could consider service name + port label as kind of primary key and tags as metadata (at least that is how I saw it).

Thinking about it all I like my approach better wrt to the outcome and yours better with regard to simplicity. I have one idea which could make yours even better: Make the scoring actually a score. With that I mean that we give the position match a score of one and name and port label each a score of 2 and we accept the index with the highest score. I'll try to put that into code -- what do you think?

[apollo13]

I came up with the following diff for your PR:

diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go
index 72ccadb0b..e9f9fd453 100644
--- a/nomad/structs/diff.go
+++ b/nomad/structs/diff.go
@@ -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 slight preference
+		// towards name + portlabel over index position.
 		score := 0
 		if i == serviceIndex {
 			score += 1
 		}
 
 		if service.Name == s.Name {
-			score += 1
+			score += 2
 		}
 
 		if service.PortLabel == s.PortLabel {
-			score += 1
+			score += 2
 		}
 
-		if score >= 2 {
-			return i
+		if score > 0 && 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 c1c04af76..ad7d4ca6c 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{

What do you think?

[apollo13]

Oh, with your changes we have some failures in TestTaskDiff. I'll see if we need to adjust the tests there or if we did fundamentally break it :d

[apollo13]

Okay, so I have fixed up the failures in the existing tests and pushed your (and my new changes) into this branch. WDYT?

[lgfa29]

Looks good! Thanks for the extra testing and code.

I just pushed a few minor improvements to try and make some of the values in findServiceMatch more clear. I also noticed that I accidentally removed the line that sorted diffs before returning and that the last for loop was not actually required, since we were already looping over the old services list.

[apollo13]

Thank you for the cleanups, I've left an inline comment regarding the testfailure. Sorting of the diffs also make sense and is better than my naive reordering to fix the tests (I'll blame my unfamiliarity with the nomad codebase).

[lgfa29]

I going to go ahead and merge this as we have tested that this results in a better output than the current code.

Thank you @apollo13 🎉

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.