Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: remove internalization of affinity strings #10904

Merged
merged 1 commit into from
Jul 22, 2021
Merged

core: remove internalization of affinity strings #10904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/10897.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
core: Fixed a bug where affinity memoization may cause planning problems
```
12 changes: 0 additions & 12 deletions nomad/structs/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
Expand All @@ -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",
},
},
},
Expand Down
18 changes: 8 additions & 10 deletions nomad/structs/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -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,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm conflicted about this transition in this PR and the constraints PR. The previous way copes with changes to the structs better and is more common approach in this file; but I like the explicitness here. Would like to know how you view it and whether we should change the rest of Copy functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it comes down to which kind of bug(s) do you want to track down in the future. The explicit assignment here implies you'll have a zero-value or a nil-value if you forget to add the new field.

OTOH using the de-reference copy trick creates a shallow copy - which if you add a pointer field and forget to deep copy it, it will now be shared and mutable by either owner.

I my experience NPE's and zero-values have been easier to track down than shared mutable state, which is why I kind of prefer these explicit copies.

Maybe the best thing would actually be to use something like https://pkg.go.dev/github.com/getlantern/deepcopy, or have Go add copy constructors to the language

}

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 {
Expand Down
18 changes: 12 additions & 6 deletions nomad/structs/structs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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},
}))
}

Expand Down