-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Ensure allocDir is never nil and persisted safely #2838
Conversation
3f7919d
to
bf5c18d
Compare
client/alloc_runner.go
Outdated
} | ||
// Build allocation directory (idempotent) | ||
if err := r.allocDir.Build(); err != nil { | ||
r.logger.Printf("[WARN] client: failed to build task directories: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it [ERROR]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Thanks. Fixing.
Task dir metadata is created in AllocRunner.Run which may not run before an alloc is sync'd and Nomad exits. There's no reason not to just create task dir metadata on restore if it doesn't exist.
Since the AllocRunner.alloc struct can be mutated, most of AllocRunner needs to acquire a lock to get the alloc's ID. Log lines always need to include the alloc ID, so we often skipped acquiring a lock just to grab the ID and accepted the race. Let's make the race detector a little happier by storing the ID in a single assignment field.
860a9ed
to
b0eae2f
Compare
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. |
Fixes #2834
Sorry for the PR noise by adding the
AllocRunner.allocID
field. It was part of a larger effort I mostly abandoned but wanted to keep that bit.