core: remove internalization of affinity strings

Basically the same as #10896 but with the Affinity struct. Since we use reflect.DeepEquals for job comparison, there is risk of false positives for changes due to a job struct with memoized vs non-memoized strings. Closes #10897
hashicorp · Jul 15, 2021 · aa86392 · aa86392
1 parent 428174a
commit aa86392
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 28 deletions.
diff --git a/.changelog/10897.txt b/.changelog/10897.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+core: Fixed a bug where affinity memoization may cause planning problems
+```
diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
@@ -761,14 +761,12 @@ func TestJobDiff(t *testing.T) {
 						RTarget: "foo",
 						Operand: "foo",
 						Weight:  20,
-						str:     "foo",
 					},
 					{
 						LTarget: "bar",
 						RTarget: "bar",
 						Operand: "bar",
 						Weight:  20,
-						str:     "bar",
 					},
 				},
 			},
@@ -779,14 +777,12 @@ func TestJobDiff(t *testing.T) {
 						RTarget: "foo",
 						Operand: "foo",
 						Weight:  20,
-						str:     "foo",
 					},
 					{
 						LTarget: "baz",
 						RTarget: "baz",
 						Operand: "baz",
 						Weight:  20,
-						str:     "baz",
 					},
 				},
 			},
@@ -1558,14 +1554,12 @@ func TestTaskGroupDiff(t *testing.T) {
 						RTarget: "foo",
 						Operand: "foo",
 						Weight:  20,
-						str:     "foo",
 					},
 					{
 						LTarget: "bar",
 						RTarget: "bar",
 						Operand: "bar",
 						Weight:  20,
-						str:     "bar",
 					},
 				},
 			},
@@ -1576,14 +1570,12 @@ func TestTaskGroupDiff(t *testing.T) {
 						RTarget: "foo",
 						Operand: "foo",
 						Weight:  20,
-						str:     "foo",
 					},
 					{
 						LTarget: "baz",
 						RTarget: "baz",
 						Operand: "baz",
 						Weight:  20,
-						str:     "baz",
 					},
 				},
 			},
@@ -4192,14 +4184,12 @@ func TestTaskDiff(t *testing.T) {
 						RTarget: "foo",
 						Operand: "foo",
 						Weight:  20,
-						str:     "foo",
 					},
 					{
 						LTarget: "bar",
 						RTarget: "bar",
 						Operand: "bar",
 						Weight:  20,
-						str:     "bar",
 					},
 				},
 			},
@@ -4210,14 +4200,12 @@ func TestTaskDiff(t *testing.T) {
 						RTarget: "foo",
 						Operand: "foo",
 						Weight:  20,
-						str:     "foo",
 					},
 					{
 						LTarget: "baz",
 						RTarget: "baz",
 						Operand: "baz",
 						Weight:  20,
-						str:     "baz",
 					},
 				},
 			},

diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
@@ -8366,10 +8366,9 @@ type Affinity struct {
 	RTarget string // Right-hand target
 	Operand string // Affinity operand (<=, <, =, !=, >, >=), set_contains_all, set_contains_any
 	Weight  int8   // Weight applied to nodes that match the affinity. Can be negative
-	str     string // Memoized string
 }
 
-// Equal checks if two affinities are equal
+// Equals checks if two affinities are equal.
 func (a *Affinity) Equals(o *Affinity) bool {
 	return a == o ||
 		a.LTarget == o.LTarget &&
@@ -8386,17 +8385,16 @@ func (a *Affinity) Copy() *Affinity {
 	if a == nil {
 		return nil
 	}
-	na := new(Affinity)
-	*na = *a
-	return na
+	return &Affinity{
+		LTarget: a.LTarget,
+		RTarget: a.RTarget,
+		Operand: a.Operand,
+		Weight:  a.Weight,
+	}
 }
 
 func (a *Affinity) String() string {
-	if a.str != "" {
-		return a.str
-	}
-	a.str = fmt.Sprintf("%s %s %s %v", a.LTarget, a.Operand, a.RTarget, a.Weight)
-	return a.str
+	return fmt.Sprintf("%s %s %s %v", a.LTarget, a.Operand, a.RTarget, a.Weight)
 }
 
 func (a *Affinity) Validate() error {

diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go
@@ -284,6 +284,12 @@ func TestJob_SpecChanged(t *testing.T) {
 			Original: &Job{Constraints: []*Constraint{{"A", "B", "="}}},
 			New:      &Job{Constraints: []*Constraint{{"A", "B", "="}}},
 		},
+		{
+			Name:     "With Affinities",
+			Changed:  false,
+			Original: &Job{Affinities: []*Affinity{{"A", "B", "=", 1}}},
+			New:      &Job{Affinities: []*Affinity{{"A", "B", "=", 1}}},
+		},
 	}
 
 	for _, c := range cases {
@@ -814,14 +820,14 @@ func TestJob_PartEqual(t *testing.T) {
 	}))
 
 	as := &Affinities{
-		&Affinity{"left0", "right0", "=", 0, ""},
-		&Affinity{"left1", "right1", "=", 0, ""},
-		&Affinity{"left2", "right2", "=", 0, ""},
+		&Affinity{"left0", "right0", "=", 0},
+		&Affinity{"left1", "right1", "=", 0},
+		&Affinity{"left2", "right2", "=", 0},
 	}
 	require.True(t, as.Equals(&Affinities{
-		&Affinity{"left0", "right0", "=", 0, ""},
-		&Affinity{"left2", "right2", "=", 0, ""},
-		&Affinity{"left1", "right1", "=", 0, ""},
+		&Affinity{"left0", "right0", "=", 0},
+		&Affinity{"left2", "right2", "=", 0},
+		&Affinity{"left1", "right1", "=", 0},
 	}))
 }