replace worker vmem config item with individual process counts #3337
Conversation
MoralCode
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| logger.info(f"The API is available at '{api_response.json()['route']}'") | ||
|
|
||
| processes = start_celery_worker_processes(float(worker_vmem_cap), disable_collection) | ||
| processes = start_celery_worker_processes((core_worker_count, secondary_worker_count, facade_worker_count), disable_collection) |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0621: Redefining name 'processes' from outer scope (line 479) (redefined-outer-name)
|
given that this had to be updated in two places, I'm also curious if one of the functions can be entirely removed. Also this probably still needs updating to adjust the place in the code where the tasks are actually added to the queues |
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
@MoralCode : Have you at least started this on your dev machine and run it ?... Ordinarily I assume that, but unwinding these changes could amount to a bunch of work if it breaks anything. .. so, asking even though I suspect the answer is "yes". :)
|
@MoralCode : ah .. looks like this one is breaking docker builds. |
MoralCode
force-pushed
the
remove-worker-vmem
branch
2 times, most recently
from
dfaa582 to
fe7a40e
Compare
MoralCode
force-pushed
the
remove-worker-vmem
branch
2 times, most recently
from
4f8fa26 to
938071c
Compare
|
@MoralCode : What are thinking re: docker build failure? Wait to merge until it passes, or solve it on another PR? |
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
Do we need to add rows to the config table? Seems like it?
| os.environ["AUGUR_DISABLE_COLLECTION"] = "1" | ||
|
|
||
| worker_vmem_cap = get_value("Celery", 'worker_process_vmem_cap') | ||
| core_worker_count = get_value("Celery", 'core_worker_count') |
There was a problem hiding this comment.
Do we need to add values to the database?
There was a problem hiding this comment.
yes, but thats why i filed #3356 - the values and sensible defaults already exist in the config.py file, and I think we should have the application insert them if something tries to access them and it cannot find the value. Then users dont need to run some manual command and we can keep the DB migrations limited to only structural fixes
MoralCode
added
the
config
MoralCode
force-pushed
the
remove-worker-vmem
branch
from
938071c to
f371f54
Compare
|
Works on my machine (and also with augur.json - although with some fiddling because podman permissions). |
MoralCode
added
the
ready
|
Docker image build still failing, @MoralCode |
|
Rerunning. cant see an obvious error besides failing to clone chaoss/whitepaper..... |
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
I think we have worked this one pretty close to the nub ... @MoralCode : Any reservations about merging?
nope, good to merge! |
…r each thing This is a tuple so it can potentially scale up without impacting the method signature Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
Signed-off-by: Adrian Edwards <adredwar@redhat.com>
MoralCode
force-pushed
the
remove-worker-vmem
branch
from
f371f54 to
267b903
Compare
2 participants
Description
This pr removes the
worker_process_vmem_capconfig item and replaces it with a configurable count per worker:core_worker_countsecondary_worker_countfacade_worker_countWe default these values to 5 workers each because we anticipate most users getting started with docker will be likely running on laptops. We plan to document that these values should be changed in production as part of that issue ( #3249 )
This makes managing the tasks more intuitive for augur admins.
Notes for Reviewers
When running this, you will need to ensure that the database values are reset based on the hardcoded defaults in order for these values to exist in your config table. There is no migration for adding this and I do not plan to add one.
Signed commits