Nomad plan does not show diff for check args

[apollo13]

Nomad version

Nomad v1.2.0-dev (efa9d4b+CHANGES)

Issue

Nomad plan doesn't show a diff if service check arguments change

Reproduction steps

Add a service to a job:

    service {
      name = "test"
      port = "test"
      check {
          type = "script"
          command = "/bin/sh" 
          args = ["-c", "echo 123"]
          interval = "30s"
          timeout = "1s"
      }
    }

Now change the args part and run nomad job plan. It will show that there is a diff in the check but it will not show that it is in args.

I started working on a simple patch, but I am not sure what the best approach to diff a sorted list (not a set) is.

diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go
index 24d1f242e..1a23a3d09 100644
--- a/nomad/structs/diff.go
+++ b/nomad/structs/diff.go
@@ -698,6 +698,11 @@ func serviceCheckDiff(old, new *ServiceCheck, contextual bool) *ObjectDiff {
        // Diff the primitive fields.
        diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, contextual)
 
+       // Args diffs
+       if arrayDiff := stringArrayDiff(old.Args, new.Args, "Args", contextual); arrayDiff != nil {
+               diff.Objects = append(diff.Objects, arrayDiff)
+       }
+
        // Diff Header
        if headerDiff := checkHeaderDiff(old.Header, new.Header, contextual); headerDiff != nil {
                diff.Objects = append(diff.Objects, headerDiff)
@@ -2221,6 +2226,55 @@ func fieldDiffs(old, new map[string]string, contextual bool) []*FieldDiff {
        return diffs
 }
 
+// stringArrayDiff diffs to array of strings with the given name.
+func stringArrayDiff(old, new []string, name string, contextual bool) *ObjectDiff {
+       deepEqual := reflect.DeepEqual(old, new)
+       if deepEqual && !contextual {
+               return nil
+       }
+
+       diff := &ObjectDiff{Name: name}
+       var added, removed bool
+
+       // Only added args
+       if len(old) == 0 && len(new) > 0 {
+               for f := range new {
+                       diff.Fields = append(diff.Fields, fieldDiff("", new[f], new[f], contextual))
+               }
+               added = true
+       } else if len(new) == 0 && len(old) > 0 { // Only removed args
+               for f := range old {
+                       diff.Fields = append(diff.Fields, fieldDiff(old[f], "", old[f], contextual))
+               }
+               removed = true
+       } else if !deepEqual { // remove all existing once and add all new oces
+               added = true
+               removed = true
+               for f := range old {
+                       diff.Fields = append(diff.Fields, fieldDiff("", old[f], old[f], contextual))
+               }
+               for f := range new {
+                       diff.Fields = append(diff.Fields, fieldDiff(new[f], "", new[f], contextual))
+               }
+       }
+
+       // Determine the type
+       if added && removed {
+               diff.Type = DiffTypeEdited
+       } else if added {
+               diff.Type = DiffTypeAdded
+       } else if removed {
+               diff.Type = DiffTypeDeleted
+       } else {
+               if len(diff.Fields) == 0 {
+                       return nil
+               }
+               diff.Type = DiffTypeNone
+       }
+
+       return diff
+}
+
 // stringSetDiff diffs two sets of strings with the given name.
 func stringSetDiff(old, new []string, name string, contextual bool) *ObjectDiff {
        oldMap := make(map[string]struct{}, len(old))

[tgross]

@apollo13 and @lgfa29 is this issue now closed by #11378 #11400 or is there more to do?

[apollo13]

Yes more to do. Currently check args are simply not diffed at all. We are slowly improving plan output all over the place :)

[tgross]

Thanks for the context @apollo13.

Probably related: #10564

[apollo13]

Indeed, those are the same. Guess we can close one of them.

…

On Mon, Nov 8, 2021, at 17:55, Tim Gross wrote: Thanks for the context @apollo13 <https://github.com/apollo13>. Probably related: #10564 <#10564> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C6UCXFT7SY7M6HEUATUK76JZANCNFSM5FIZ7IFQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[apollo13]

@lgfa29 Can you provide any pointers on how to properly show sorted lists in the diff output (you can try my patch)?

[apollo13]

@lgfa29 Mind taking a look here?

[lgfa29]

Oh sorry @apollo13, I thought I had replied to this 😳

Your code changes looks like a good start! Using fieldDiff has the side-effect of the output being a bit repetitive, but I think that's OK for a first implementation (much better that no output, that's for sure 😅)

It also has the problem of the all-or-nothing diff we cover before in other places, meaning that any change will result in a full add/remove set which, again, I think it's OK for now.

If you do feel more adventurous and would like to generate more precise diffs, you could look into the Levenshtein distance algorithm, which is often used for things like auto-correct systems (before ML and AI took over 😬)

HCL uses https://github.com/agext/levenshtein, which means it's already an indirect dependency for Nomad. But these libraries usually only work for strings, not slices. It may be possible to join the args slice, compute the distance and then map it back to a slice, but it may not be straightforward.

@angrycub found another library that does work in slices, but is marked as experimental, and seems more like an exploration side-project.

But long story short, I think your patch looks good (minor nit-pick would be to replace stringArrayDiff with stringSliceDiff), so feel free to move it as a PR 🙂

[apollo13]

Your code changes looks like a good start! Using fieldDiff has the side-effect of the output being a bit repetitive, but I think that's OK for a first implementation (much better that no output, that's for sure sweat_smile)

I know, any idea what else I could use? I didn't really find another example in nomad on a quick glance.

Looking through diffing options https://github.com/r3labs/diff looks somewhat maintained?

[apollo13]

As an example;

package main

import (
	"fmt"

	"github.com/r3labs/diff/v2"
)

func main() {
	from := []string{"-c", "a", "test", "d"}
	to := []string{"-d", "test", "b"}
	changelog, _ := diff.Diff(from, to)
	fmt.Printf("%+v\n", changelog)
}

yields:

[
{Type:update Path:[0] From:-c To:-d parent:<nil>}
{Type:delete Path:[1] From:a To:<nil> parent:<nil>}
{Type:delete Path:[3] From:d To:<nil> parent:<nil>}
{Type:create Path:[2] From:<nil> To:b parent:<nil>}
]

which seems rather reasonable. Although I am not yet sure why it thinks it needs to delete/create the last elementinstead of updating it scratches head

[lgfa29]

I know, any idea what else I could use? I didn't really find another example in nomad on a quick glance.

Yeah, I don't think we have anything like this anywhere else. Since this is mostly a visual change, I think you could special-case the rendering of the output for the case where the field name is empty, something like this:

diff --git a/command/job_plan.go b/command/job_plan.go
index b78de85c5..8d60b4a43 100644
--- a/command/job_plan.go
+++ b/command/job_plan.go
@@ -561,11 +561,18 @@ func formatObjectDiff(diff *api.ObjectDiff, startPrefix, keyPrefix int) string {
 // the number of spaces to put infront of the value for aligning values.
 func formatFieldDiff(diff *api.FieldDiff, startPrefix, keyPrefix, valuePrefix int) string {
 	marker, _ := getDiffString(diff.Type)
-	out := fmt.Sprintf("%s%s%s%s: %s",
+	prefix := fmt.Sprintf("%s%s%s",
 		strings.Repeat(" ", startPrefix),
-		marker, strings.Repeat(" ", keyPrefix),
-		diff.Name,
-		strings.Repeat(" ", valuePrefix))
+		marker,
+		strings.Repeat(" ", keyPrefix))
+	value := strings.Repeat(" ", valuePrefix)
+
+	var out string
+	if diff.Name != "" {
+		out = fmt.Sprintf("%s%s: %s", prefix, diff.Name, value)
+	} else {
+		out = fmt.Sprintf("%s  %s", prefix, value)
+	}
 
 	switch diff.Type {
 	case "Added":

Which then would generate this output instead:

Looking through diffing options https://github.com/r3labs/diff looks somewhat maintained?

Ah cool, looks good 🙂

which seems rather reasonable. Although I am not yet sure why it thinks it needs to delete/create the last elementinstead of updating it scratches head

Hum...yeah, I'm not sure either, but it seems to detect some changes.

[apollo13]

I think you could special-case the rendering of the output for the case where the field name is empty, something like this

Great idea thanks

Hum...yeah, I'm not sure either, but it seems to detect some changes.

Yeah, not sure they are correct though (playing with some other examples I don't think I can get it to work :D)

[lgfa29]

Yeah, not sure they are correct though (playing with some other examples I don't think I can get it to work :D)

If the library doesn't work I think the original diff you had would work fine for now 👍