scheduler: annotate tasksUpdated with reason and purge DeepEquals (#1…

…6421)

* scheduler: annotate tasksUpdated with reason and purge DeepEquals

* cr: move opaque into helper

* cr: swap affinity/spread hashing for slice equal

* contributing: update checklist-jobspec with notes about struct methods

* cr: add more cases to wait config equal method

* cr: use reflect when comparing envoy config blocks

* cl: add cl

.changelog/16421.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+scheduler: remove most uses of reflection for task comparisons
+```

contributing/checklist-jobspec.md

@@ -8,8 +8,11 @@
   * New fields should be added to existing Canonicalize, Copy methods
   * Test the structs/fields via methods mentioned above
 * [ ] Add structs/fields to `nomad/structs` package
-  * `structs/` structs usually have Copy, Equals, and Validate methods
-  * Validation happens in this package and _must_ be implemented
+  * `structs/` structs usually have Copy, Equal, and Validate methods
+    * `Validate` methods in this package _must_ be implemented
+    * `Equal` methods are used when comparing one job to another (e.g. did this thing get updated?)
+    * `Copy` methods ensure modifications do not modify the copy of a job in the state store
+      * Use `slices.CloneFunc` and `maps.CloneFunc` to ensure creation of deep copies
   * Note that analogous struct field names should match with `api/` package
   * Test the structs/fields via methods mentioned above
   * Implement and test other logical methods

helper/opaque.go

@@ -0,0 +1,48 @@
+package helper
+
+import (
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"golang.org/x/exp/maps"
+)
+
+var (
+	cmpOptIgnoreUnexported = ignoreUnexportedAlways()
+	cmpOptNilIsEmpty       = cmpopts.EquateEmpty()
+	cmpOptIgnore           = cmp.Ignore()
+)
+
+// ignoreUnexportedAlways creates a cmp.Option filter that will ignore unexported
+// fields of on any/all types. It is a derivative of go-cmp.IgnoreUnexported,
+// here we do not require specifying individual types.
+//
+// reference: https://github.com/google/go-cmp/blob/master/cmp/cmpopts/ignore.go#L110
+func ignoreUnexportedAlways() cmp.Option {
+	return cmp.FilterPath(
+		func(p cmp.Path) bool {
+			sf, ok := p.Index(-1).(cmp.StructField)
+			if !ok {
+				return false
+			}
+			c := sf.Name()[0]
+			return c < 'A' || c > 'Z'
+		},
+		cmpOptIgnore,
+	)
+}
+
+// OpaqueMapsEqual compare maps[<comparable>]<any> for equality, but safely by
+// using the cmp package and ignoring un-exported types, and by treating nil/empty
+// slices and maps as equal.
+//
+// This is intended as a substitute for reflect.DeepEqual in the case of "opaque maps",
+// e.g. `map[comparable]any` - such as the case for Task Driver config or Envoy proxy
+// pass-through configuration.
+func OpaqueMapsEqual[M ~map[K]V, K comparable, V any](m1, m2 M) bool {
+	return maps.EqualFunc(m1, m2, func(a, b V) bool {
+		return cmp.Equal(a, b,
+			cmpOptIgnoreUnexported, // ignore all unexported fields
+			cmpOptNilIsEmpty,       // treat nil/empty slices as equal
+		)
+	})
+}

helper/opaque_test.go

@@ -0,0 +1,88 @@
+package helper
+
+import (
+	"testing"
+
+	"github.com/hashicorp/nomad/ci"
+	"github.com/shoenig/test/must"
+)
+
+func Test_OpaqueMapsEqual(t *testing.T) {
+	ci.Parallel(t)
+
+	type public struct {
+		F int
+	}
+
+	type private struct {
+		g int
+	}
+
+	type mix struct {
+		F int
+		g int
+	}
+
+	cases := []struct {
+		name string
+		a, b map[string]any
+		exp  bool
+	}{{
+		name: "both nil",
+		a:    nil,
+		b:    nil,
+		exp:  true,
+	}, {
+		name: "empty and nil",
+		a:    nil,
+		b:    make(map[string]any),
+		exp:  true,
+	}, {
+		name: "same strings",
+		a:    map[string]any{"a": "A"},
+		b:    map[string]any{"a": "A"},
+		exp:  true,
+	}, {
+		name: "same public struct",
+		a:    map[string]any{"a": &public{F: 42}},
+		b:    map[string]any{"a": &public{F: 42}},
+		exp:  true,
+	}, {
+		name: "different public struct",
+		a:    map[string]any{"a": &public{F: 42}},
+		b:    map[string]any{"a": &public{F: 10}},
+		exp:  false,
+	}, {
+		name: "different private struct",
+		a:    map[string]any{"a": &private{g: 42}},
+		b:    map[string]any{"a": &private{g: 10}},
+		exp:  true, // private fields not compared
+	}, {
+		name: "mix same public different private",
+		a:    map[string]any{"a": &mix{F: 42, g: 1}},
+		b:    map[string]any{"a": &mix{F: 42, g: 2}},
+		exp:  true, // private fields not compared
+	}, {
+		name: "mix different public same private",
+		a:    map[string]any{"a": &mix{F: 42, g: 1}},
+		b:    map[string]any{"a": &mix{F: 10, g: 1}},
+		exp:  false,
+	}, {
+		name: "nil vs empty slice values",
+		a:    map[string]any{"key": []string(nil)},
+		b:    map[string]any{"key": make([]string, 0)},
+		exp:  true,
+	}, {
+		name: "nil vs empty map values",
+		a:    map[string]any{"key": map[int]int(nil)},
+		b:    map[string]any{"key": make(map[int]int, 0)},
+		exp:  true,
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := OpaqueMapsEqual(tc.a, tc.b)
+			must.Eq(t, tc.exp, result)
+		})
+	}
+}

nomad/structs/csi.go

@@ -80,6 +80,25 @@ type TaskCSIPluginConfig struct {
 	HealthTimeout time.Duration `mapstructure:"health_timeout" hcl:"health_timeout,optional"`
 }
 
+func (t *TaskCSIPluginConfig) Equal(o *TaskCSIPluginConfig) bool {
+	if t == nil || o == nil {
+		return t == o
+	}
+	switch {
+	case t.ID != o.ID:
+		return false
+	case t.Type != o.Type:
+		return false
+	case t.MountDir != o.MountDir:
+		return false
+	case t.StagePublishBaseDir != o.StagePublishBaseDir:
+		return false
+	case t.HealthTimeout != o.HealthTimeout:
+		return false
+	}
+	return true
+}
+
 func (t *TaskCSIPluginConfig) Copy() *TaskCSIPluginConfig {
 	if t == nil {
 		return nil

nomad/structs/csi_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/nomad/ci"
+	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/require"
 )
 
@@ -1033,3 +1034,33 @@ func TestDeleteNodeForType_Monolith_NilNode(t *testing.T) {
 	_, ok = plug.Controllers["foo"]
 	require.False(t, ok)
 }
+
+func TestTaskCSIPluginConfig_Equal(t *testing.T) {
+	ci.Parallel(t)
+
+	must.Equal[*TaskCSIPluginConfig](t, nil, nil)
+	must.NotEqual[*TaskCSIPluginConfig](t, nil, new(TaskCSIPluginConfig))
+
+	must.StructEqual(t, &TaskCSIPluginConfig{
+		ID:                  "abc123",
+		Type:                CSIPluginTypeMonolith,
+		MountDir:            "/opt/csi/mount",
+		StagePublishBaseDir: "/base",
+		HealthTimeout:       42 * time.Second,
+	}, []must.Tweak[*TaskCSIPluginConfig]{{
+		Field: "ID",
+		Apply: func(c *TaskCSIPluginConfig) { c.ID = "def345" },
+	}, {
+		Field: "Type",
+		Apply: func(c *TaskCSIPluginConfig) { c.Type = CSIPluginTypeNode },
+	}, {
+		Field: "MountDir",
+		Apply: func(c *TaskCSIPluginConfig) { c.MountDir = "/csi" },
+	}, {
+		Field: "StagePublishBaseDir",
+		Apply: func(c *TaskCSIPluginConfig) { c.StagePublishBaseDir = "/opt/base" },
+	}, {
+		Field: "HealthTimeout",
+		Apply: func(c *TaskCSIPluginConfig) { c.HealthTimeout = 1 * time.Second },
+	}})
+}

nomad/structs/services.go

@@ -1201,8 +1201,8 @@ func (t *SidecarTask) Equal(o *SidecarTask) bool {
 		return false
 	}
 
-	// config compare
-	if !opaqueMapsEqual(t.Config, o.Config) {
+	// task config, use opaque maps equal
+	if !helper.OpaqueMapsEqual(t.Config, o.Config) {
 		return false
 	}
 
@@ -1378,15 +1378,6 @@ func (p *ConsulProxy) Copy() *ConsulProxy {
 	}
 }
 
-// opaqueMapsEqual compares map[string]interface{} commonly used for opaque
-// config blocks. Interprets nil and {} as the same.
-func opaqueMapsEqual(a, b map[string]interface{}) bool {
-	if len(a) == 0 && len(b) == 0 {
-		return true
-	}
-	return reflect.DeepEqual(a, b)
-}
-
 // Equal returns true if the structs are recursively equal.
 func (p *ConsulProxy) Equal(o *ConsulProxy) bool {
 	if p == nil || o == nil {
@@ -1409,7 +1400,8 @@ func (p *ConsulProxy) Equal(o *ConsulProxy) bool {
 		return false
 	}
 
-	if !opaqueMapsEqual(p.Config, o.Config) {
+	// envoy config, use reflect
+	if !reflect.DeepEqual(p.Config, o.Config) {
 		return false
 	}
 
@@ -1504,7 +1496,8 @@ func (u *ConsulUpstream) Equal(o *ConsulUpstream) bool {
 		return false
 	case !u.MeshGateway.Equal(o.MeshGateway):
 		return false
-	case !opaqueMapsEqual(u.Config, o.Config):
+	case !reflect.DeepEqual(u.Config, o.Config):
+		// envoy config, use reflect
 		return false
 	}
 	return true
@@ -1790,7 +1783,8 @@ func (p *ConsulGatewayProxy) Equal(o *ConsulGatewayProxy) bool {
 		return false
 	}
 
-	if !opaqueMapsEqual(p.Config, o.Config) {
+	// envoy config, use reflect
+	if !reflect.DeepEqual(p.Config, o.Config) {
 		return false
 	}