From 62d2a701b4236a8c7aff4cd769775fa77c6da014 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 14 Jul 2021 11:22:49 -0500 Subject: [PATCH] Merge pull request #10896 from hashicorp/b-rm-constraint-memoization core: do not internalize constraint strings --- .changelog/10896.txt | 3 +++ nomad/structs/diff_test.go | 12 ------------ nomad/structs/structs.go | 18 ++++++++---------- nomad/structs/structs_test.go | 18 ++++++++++++------ 4 files changed, 23 insertions(+), 28 deletions(-) create mode 100644 .changelog/10896.txt diff --git a/.changelog/10896.txt b/.changelog/10896.txt new file mode 100644 index 000000000000..6dc58d3204e8 --- /dev/null +++ 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 index 4056b417818f..476193192ed5 100644 --- 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 index 4204a6d3e27a..f56e61c503b4 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -8105,10 +8105,9 @@ 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 && @@ -8116,6 +8115,7 @@ func (c *Constraint) Equals(o *Constraint) bool { 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 index 4787c5832d43..667193a7a733 100644 --- 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{