Add records for Api app status #4249
Conversation
mdellweg
force-pushed
the
3175_api_worker_records
branch
3 times, most recently
from
b468fd9
to
30e5d64
Compare
|
The current version of the change required, that gunicorn is started with "--worker-class pulpcore.app.gunicorn_worker.PulpApiWorker`. |
|
I think this is a good way to solve this problem. +1 to continuing this approach. |
mdellweg
force-pushed
the
3175_api_worker_records
branch
from
30e5d64
to
65ae2f4
Compare
|
I redesigned and pulpcore will now provide a |
mdellweg
force-pushed
the
3175_api_worker_records
branch
2 times, most recently
from
a009185
to
e8a273f
Compare
mdellweg
force-pushed
the
3175_api_worker_records
branch
from
e8a273f
to
1ab1d16
Compare
pulpcore/api_gunicorn.py
Outdated
| def gunicorn(): | ||
| PulpcoreApiApplication("%(prog)s [OPTIONS]").run() |
There was a problem hiding this comment.
I need confirmation that we want to start running our components this way.
So if two @pulp/core devs could ack this specific part please?
edit: Even if not technically needed today, should we have the same for pulpcore-content?
There was a problem hiding this comment.
This question is especially important for the decision on this PR: pulp/pulp-oci-images#532
There was a problem hiding this comment.
"confirmation that we want to start running our components this way."
I guess my only real question is, is there some downside to this? I can't think of one, but I'm prob just being dense.
There was a problem hiding this comment.
Maybe that it's another layer of abstraction. But that goes on the plus side as well. When i think about our plan to redesign the content app, it will be much easier to just change the gunicorn worker type used in the entrypoint than telling people to modify their service files.
And exactly that thought brings me to the next question: How much should our entrypoint mimick the gunicorn commandline? Should we just override some parameters internally, or should we explicitely proxy only some parameters?
|
We need to make sure that users can still specify gunicorn timeout, number of workers, and logging format. |
mdellweg
force-pushed
the
3175_api_worker_records
branch
2 times, most recently
from
c0c1f6f
to
b94b3d2
Compare
mdellweg
force-pushed
the
3175_api_worker_records
branch
13 times, most recently
from
1987899
to
ccf4530
Compare
mdellweg
requested review from
a team and
decko
and removed request for
a team
mdellweg
force-pushed
the
3175_api_worker_records
branch
from
ccf4530
to
f1cdce8
Compare
| if self.api_app_status.versions != self.versions: | ||
| self.api_app_status.versions = self.versions | ||
| self.api_app_status.save(update_fields=["versions"]) |
There was a problem hiding this comment.
Is it really possible for the versions to update from underneath itself? The name should change upon restart and the previous record should be deleted, no?
There was a problem hiding this comment.
In theory yes. The name is composed of <pid>@<hostname> when you restart that computer, and the boot process is very consistent, you will end up with the same name again.
| # cleanup | ||
| if self.api_app_status: | ||
| self.api_app_status.delete() |
There was a problem hiding this comment.
Is it possible we could close/kill the worker and not delete the record?
There was a problem hiding this comment.
Why should we? Also there was a request from @bmbouter when implementing the hearbeating for pulpcore-workers to clean up on exiting. And this PR tries to apply these rules to all types of workers.
And btw we never cleaned up the content api status before. I guess there are some db's around where that table became humongous by now.
There was a problem hiding this comment.
Oh sorry, I worded my question wrong. I was asking is it possible the worker could be kill and not have its record cleaned up. Looking later down in the code, it seems that the task worker code would clean up any missing worker records.
There was a problem hiding this comment.
The cleanup happens after a week (or so). But yes, it's always possible that the application is killed in a way that no cleanup procedures will be executed anymore. OOM, kill -9, segfault, you name it...
There was a problem hiding this comment.
Can you link the appropriate gunicorn documentation for the classes we import and options we are allowed to pass in?
| async def _heartbeat_ctx(app): | ||
| heartbeat_task = asyncio.create_task(_heartbeat()) | ||
| yield | ||
| heartbeat_task.cancel() |
There was a problem hiding this comment.
Nifty, this should remove the asyncio Error 'Task was destroy but it is pending' from the logs when restarting the content app?
pulpcore/app/models/status.py
Outdated
| @@ -56,9 +76,7 @@ def online(self): | |||
| Returns: | |||
| bool: True if the content app is considered online, otherwise False | |||
There was a problem hiding this comment.
s/content app/app. Also, you need to change the docstring for the missing property as well.
mdellweg
force-pushed
the
3175_api_worker_records
branch
2 times, most recently
from
7114284
to
3598075
Compare
mdellweg
force-pushed
the
3175_api_worker_records
branch
from
1514041
to
b30ca00
Compare
fixes #3175