Merge pull request #10896 from hashicorp/b-rm-constraint-memoization

core: do not internalize constraint strings
hashicorp · Jul 29, 2021 · c13b770 · c13b770
1 parent 2d1ed12
commit c13b770
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 28 deletions.
diff --git a/.changelog/10896.txt b/.changelog/10896.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+core: Fixed a bug where internalized constraint strings broke job plan
+```
diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
@@ -676,13 +676,11 @@ func TestJobDiff(t *testing.T) {
 						LTarget: "foo",
 						RTarget: "foo",
 						Operand: "foo",
-						str:     "foo",
 					},
 					{
 						LTarget: "bar",
 						RTarget: "bar",
 						Operand: "bar",
-						str:     "bar",
 					},
 				},
 			},
@@ -692,13 +690,11 @@ func TestJobDiff(t *testing.T) {
 						LTarget: "foo",
 						RTarget: "foo",
 						Operand: "foo",
-						str:     "foo",
 					},
 					{
 						LTarget: "baz",
 						RTarget: "baz",
 						Operand: "baz",
-						str:     "baz",
 					},
 				},
 			},
@@ -1477,13 +1473,11 @@ func TestTaskGroupDiff(t *testing.T) {
 						LTarget: "foo",
 						RTarget: "foo",
 						Operand: "foo",
-						str:     "foo",
 					},
 					{
 						LTarget: "bar",
 						RTarget: "bar",
 						Operand: "bar",
-						str:     "bar",
 					},
 				},
 			},
@@ -1493,13 +1487,11 @@ func TestTaskGroupDiff(t *testing.T) {
 						LTarget: "foo",
 						RTarget: "foo",
 						Operand: "foo",
-						str:     "foo",
 					},
 					{
 						LTarget: "baz",
 						RTarget: "baz",
 						Operand: "baz",
-						str:     "baz",
 					},
 				},
 			},
@@ -3963,13 +3955,11 @@ func TestTaskDiff(t *testing.T) {
 						LTarget: "foo",
 						RTarget: "foo",
 						Operand: "foo",
-						str:     "foo",
 					},
 					{
 						LTarget: "bar",
 						RTarget: "bar",
 						Operand: "bar",
-						str:     "bar",
 					},
 				},
 			},
@@ -3979,13 +3969,11 @@ func TestTaskDiff(t *testing.T) {
 						LTarget: "foo",
 						RTarget: "foo",
 						Operand: "foo",
-						str:     "foo",
 					},
 					{
 						LTarget: "baz",
 						RTarget: "baz",
 						Operand: "baz",
-						str:     "baz",
 					},
 				},
 			},

diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
@@ -8105,17 +8105,17 @@ type Constraint struct {
 	LTarget string // Left-hand target
 	RTarget string // Right-hand target
 	Operand string // Constraint operand (<=, <, =, !=, >, >=), contains, near
-	str     string // Memoized string
 }
 
-// Equal checks if two constraints are equal
+// Equals checks if two constraints are equal.
 func (c *Constraint) Equals(o *Constraint) bool {
 	return c == o ||
 		c.LTarget == o.LTarget &&
 			c.RTarget == o.RTarget &&
 			c.Operand == o.Operand
 }
 
+// Equal is like Equals but with one less s.
 func (c *Constraint) Equal(o *Constraint) bool {
 	return c.Equals(o)
 }
@@ -8124,17 +8124,15 @@ func (c *Constraint) Copy() *Constraint {
 	if c == nil {
 		return nil
 	}
-	nc := new(Constraint)
-	*nc = *c
-	return nc
+	return &Constraint{
+		LTarget: c.LTarget,
+		RTarget: c.RTarget,
+		Operand: c.Operand,
+	}
 }
 
 func (c *Constraint) String() string {
-	if c.str != "" {
-		return c.str
-	}
-	c.str = fmt.Sprintf("%s %s %s", c.LTarget, c.Operand, c.RTarget)
-	return c.str
+	return fmt.Sprintf("%s %s %s", c.LTarget, c.Operand, c.RTarget)
 }
 
 func (c *Constraint) Validate() error {

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