-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Infinite memory growth in Nomad Agent and Nomad Client when using batch jobs. #15090
Comments
Having though about this more and looked into more code, I think the fix might be as simple as comparing the That said, the code is internally inconsistent. This test seems to assert that we should never GC allocs or evals inbetween runs: nomad/nomad/core_sched_test.go Lines 1298 to 1312 in 2b054e3
However, this code seems to indicate something very different: Line 339 in 2b054e3
|
Thanks for the report and the PR @stswidwinski 😄 I've been looking into this but I'm not 100% sure of the details yet. At first sight your analysis makes a lot of sense, in if alloc.Job != nil && alloc.Job.CreateIndex < job.CreateIndex && alloc.TerminalStatus() {
# ...
}
Any additional details about your test environment that you could provide? Like how many servers and clients are you using? |
Sure! I'll write up a detailed repro tomorrow.
…On Mon, Nov 7, 2022, 7:05 PM Luiz Aoqui ***@***.***> wrote:
Thanks for the report and the PR @stswidwinski
<https://github.com/stswidwinski> 😄
I've been looking into this but I'm not 100% sure of the details yet. At
first sight your analysis makes a lot of sense, in
if alloc.Job != nil && alloc.Job.CreateIndex < job.CreateIndex && alloc.TerminalStatus() {
# ...
}
alloc.Job.CreateIndex < job.CreateIndex seems like it should always be
false as you mentioned, but when I tried to reproduce the problem, the GC
worked and removed the batch job allocs and evals.
Any additional details about your test environment that you could provide?
Like how many servers and clients are you using?
—
Reply to this email directly, view it on GitHub
<#15090 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AECDANH66F3QA2C3NQQZIT3WHGKLFANCNFSM6AAAAAARTNGBAU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@lgfa29, I think I know what problem you were seeing. When the job specification does not change between updates the former allocation is reused to reduce the churn within a fleet. However, any change to the job spec will induce a new allocation. However, even without changes to the spec one may easily see the agglomeration of the evaluations in perpetuity. Here is a repro starting with installation of the nomad RPM and going all the way to tracking evaluations and allocations in time. Setup:
Test script:
Test run:
Observe the output (I truncated the middle 90 rows since they aren't particularly interesting (they're exactly as one would assume):
Please notice that:
I have previously let this sit around for 30 more minutes and the outcome is the same. I have ran this over a week on a tiny deployment with the same results (except much larger numbers). The cluster eventually runs out of memory -- both client and scheduler. The GC limits are such that both evals and allocs should've been GCed (except for the most recent one which is necessary for no rescheduling of the tasks). Here is the output of the nomad CLI at the end of the test for a summary of the job information in case it is helpful:
|
Hi @stswidwinski, apologies for the delay here, I was going through internal documents and conversations to understand why the change in 2409f72 was needed, and I think it relates to a specific issue at large clusters, but I haven't been able to find much information yet. On a first pass I think your code changes make sense, but I want to make sure we're not causing some edge case regression that was supposed to be fixed by 2409f72. I will try to get back to it next week. |
@lgfa29 any luck? :) |
Hi @stswidwinski, After checking with the team, we think it may be better to leave this part of the logic as-is and introduce a new configuration value (something like To reduce the effects on existing deployments, this new configuration value should be set to something high, like The reason for this approach is that it's hard for us to tell what people expect out of the GC for batch jobs. Some case may benefit from having a long history, while others may prefer to reap them quickly. An explicit configuration value makes the behaviour more clear to users, and allow them to fine-tune it for their needs, and for us developers as well since the logic becomes more straight forward. In terms of code changes, in addition to creating and plumbing the configuration all the way to the server, I think we would only need a new What do you think of this approach? |
Hey @lgfa29, Thank you for the response and the thought process. This makes sense to me. I'll make the changes and update the PR accordingly. I hope to get to this today, but if not... I'm highly likely to do this by EOW next week. I believe that there are still some (perhaps tangential) issues with the existing logic for pruning evaluations (reference to Thanks again! |
Sounds good! Thanks for all the work you've done 🙂 |
…ows us to control the pace of GC of Batch Evals independent of other evals.
Updated the PR, added tests and pulled the configuration value from the command through the scheduler. I'm not 100% confident that this is enough given that I am relatively new to the entire codebase (awaiting test chain). Please let me know if I misunderstood your comment above and/or if there are other changes you would like to see in the PR. |
Fixed in #15097 which will ship in Nomad 1.5.0 (with backports) |
Nomad version
Nomad v1.3.1 (2b054e3)
But this is broken on
v.1.4.2
as well.Operating system and Environment details
Unix.
Issue
Evaluations and Allocations for Batch jobs are not garbage collected as long as the batch job is not stopped. The growth continues ad-infinitum causing two side-effects:
There are only two ways to remove this memory growth:
nomad system gc
(which removes any history and data)Both of which put the burden of cleanup on the end-user. Now, let me point to the exact lines of code which are responsible for the behavior and go through the logic in detail. First, the GC of evaluations and allocations for batch jobs is somewhat special-cased:
https://github.com/hashicorp/nomad/blob/main/nomad/core_sched.go#L303-L328
The evaluation is never garbage collected unless the corresponding job is fully stopped.
allowBatch
is somewhat moot because it is nevertrue
unless we force gc. Now, infinite growth of allocations would be bad, but would not cause the infinite memory growth on clients. I believe that this is the reasoning behind line 325 which attempts to GC old allocations:https://github.com/hashicorp/nomad/blob/main/nomad/core_sched.go#L347-L357
However, please notice that an allocation is never "re-parented" between different jobs and the
.Job
object is a pointer. Hence, we always get the equality ofalloc.Job == job
which yieldsalloc.Job.CreateIndex == job.CreateIndex
. Hence, this is never garbage collected. Please note that this is due to the fact that theCreateIndex
is set only once during the first raft-level serialization of the object:nomad/nomad/state/state_store.go
Line 1643 in c725c1b
The Job pointer of an allocation is similarly only ever set when the job is (de-)serialized for the first time:
nomad/nomad/state/state_store.go
Line 3641 in c725c1b
History.
Please note that this logic has been broken for quite some time. In 2018 the logic shifted from using ModifyIndex to CreateIndex:
2409f72
But the original logic still wouldn't have worked due to
Job
being a pointer rather than a value. Similar bugs have been reported in the past:#4532
#14842
Fix.
The fix here is to consider not the entire history of allocations for a batch job, but only the most recent one (or even 10 most recent ones or some other small
n
).Reproduction steps
Create a batch job with arbitrary command. Assume
/usr/bin/true
for this repro. What is important is that the type of the job remainbatch
. Then, proceed to create some number of job updates with trivial changes to the job spec which do not result in functional changes.This is to ensure that we trigger a new evaluation and a new job version -- the simplest way to achieve this is to add any random
nonce
metadata token which changes with every update. Observe infinite memory growth.Expected Result
Allocations should be garbage collected with time and the infinite history should not persist.
Actual Result
The opposite.
Job file (if appropriate)
The text was updated successfully, but these errors were encountered: