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

Jump to bottom

core: do not internalize constraint strings #10896

Merged
merged 4 commits into from Jul 14, 2021
Merged

core: do not internalize constraint strings #10896

merged 4 commits into from Jul 14, 2021

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Jul 14, 2021

This PR causes Nomad to no longer memoize the String value of
a Constraint. The private memoized variable may or may not be
initialized at any given time, which means a reflect.DeepEqual
comparison between two jobs (e.g. during Plan) may return incorrect
results.

Fixes #10836

@shoenig
core: do not memoize constraint strings
07c57ff 
This PR causes Nomad to no longer memoize the String value of
a Constraint. The private memoized variable may or may not be
initialized at any given time, which means a reflect.DeepEqual
comparison between two jobs (e.g. during Plan) may return incorrect
results.

Fixes #10836
notnoop
notnoop approved these changes Jul 14, 2021
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

Wow - that's quite a gotcha! I'd suggest adding a warning about memoized/private fields and also adding a test that triggers #10836 .

@vercel vercel bot temporarily deployed to Preview – nomad July 14, 2021 15:46 Inactive
@shoenig shoenig mentioned this pull request Jul 14, 2021
Closed
@shoenig shoenig merged commit 8d0e319 into main Jul 14, 2021
@shoenig shoenig deleted the b-rm-constraint-memoization branch July 14, 2021 16:22
shoenig added a commit that referenced this pull request Jul 15, 2021
@shoenig
core: remove internalization of affinity strings
aa86392 
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
@shoenig shoenig mentioned this pull request Jul 15, 2021
Merged
@notnoop notnoop added this to the 1.1.3 milestone Jul 28, 2021
@notnoop notnoop mentioned this pull request Jul 28, 2021
Merged
notnoop pushed a commit that referenced this pull request Jul 28, 2021
@shoenig
Merge pull request #10896 from hashicorp/b-rm-constraint-memoization
62d2a70 
core: do not internalize constraint strings
notnoop pushed a commit that referenced this pull request Jul 29, 2021
@shoenig
Merge pull request #10896 from hashicorp/b-rm-constraint-memoization
c13b770 
core: do not internalize constraint strings
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@notnoop notnoop notnoop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.3
Development

Successfully merging this pull request may close these issues.

Plan returns an erroneous in-place update in diff when task group has a constraint
4 participants
@shoenig @notnoop @schmichael @lgfa29