Consider all system jobs for a new node #11054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
August 13, 2021 10:40
When a node becomes ready, create an eval for all system jobs across namespaces. The previous code uses `job.ID` to deduplicate evals, but that ignores the job namespace. Thus if there are multiple jobs in different namespaces sharing the same ID/Name, only one will be considered for running in the new node. Thus, Nomad may skip running some system jobs in that node.
schmichael
approved these changes
Aug 13, 2021
nomad/node_endpoint.go
Outdated
| @@ -1418,10 +1418,10 @@ func (n *Node) createNodeEvals(nodeID string, nodeIndex uint64) ([]string, uint6 | |||
| // Create an evaluation for each system job. | |||
| for _, job := range sysJobs { | |||
| // Still dedup on JobID as the node may already have the system job. | |||
| if _, ok := jobIDs[job.ID]; ok { | |||
| if _, ok := jobIDs[*job.NamespacedID()]; ok { | |||
There was a problem hiding this comment.
Out of scope: Should we drop the pointer from Job.NamespacedID() *NamespacedID so it matches the behavior of structs.NewNamespacedID(...) and Allocation.JobNamespacedID()?
Derefs without nil checks just always scare me even if they're "known" to be safe through manual code analysis.
added 2 commits
August 18, 2021 09:17
NamespacedID is never nil and is very light (two string fields) that isn't in the hot path. So it's safe to return and use the struct directly. Avoiding using pointers here avoids the risk of dereferencing a nil pointer.
notnoop
pushed a commit
that referenced
this pull request
Aug 26, 2021
When a node becomes ready, create an eval for all system jobs across namespaces. The previous code uses `job.ID` to deduplicate evals, but that ignores the job namespace. Thus if there are multiple jobs in different namespaces sharing the same ID/Name, only one will be considered for running in the new node. Thus, Nomad may skip running some system jobs in that node.
notnoop
pushed a commit
that referenced
this pull request
Aug 26, 2021
When a node becomes ready, create an eval for all system jobs across namespaces. The previous code uses `job.ID` to deduplicate evals, but that ignores the job namespace. Thus if there are multiple jobs in different namespaces sharing the same ID/Name, only one will be considered for running in the new node. Thus, Nomad may skip running some system jobs in that node.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a node becomes ready, create an eval for all system jobs across
namespaces.
The previous code uses
job.IDto deduplicate evals, but that ignoresthe job namespace. Thus if there are multiple jobs in different
namespaces sharing the same ID/Name, only one will be considered for
running in the new node. Thus, Nomad may skip running some system jobs
in that node.
I've added a failing test in the first comment to highlight the issue: https://app.circleci.com/pipelines/github/hashicorp/nomad/17325/workflows/0a3cbdb9-8433-403d-b226-97eb0f9218c5/jobs/170084 that passes with the fix.