Updates to /status API #1404
Updates to /status API #1404
Conversation
| { | ||
| "api_version": "2", | ||
| "pulp_messaging_connection": true, | ||
| "pulp_database_connection": true, |
There was a problem hiding this comment.
Would we ever want to put more information on these, perhaps DB host name or detected version, or whether SSL is being used? If so, it might be good to go ahead and make these JSON objects instead of bools so that we could add to them in a Y release instead of having to wait for an X release to change them. Even if we just make it a JSON object with only the key "connected" set to true/false, I think that's future-safe.
This isn't a blocker, so do what you prefer.
There was a problem hiding this comment.
That makes sense. I will update the PR for this.
There was a problem hiding this comment.
it now does
"pulp_db_connection": {
"connected": true
},
"pulp_messaging_connection": {
"connected": true
},
"pulp_version": {
"platform_version": "2.6.0"
}
|
LGTM bro! Just watch out for that thing about which workers the resource_manager will try to assign work to. It might have just queried that known workers table, so we should make sure it avoids itself and the scheduler. |
|
@beav, let me know when you merge this. I started a PR bmbouter#3 to match the /status behavior with django. |
|
@asmacdo will do. It will not be until later this week, I need to get in a PR for another BZ first. |
|
@bmbouter noticed an unused |
|
After consultation with @bmbouter, it turned out to be better to just filter out special workers like the scheduler when we look for free workers. |
beav
force-pushed
the
status-api
branch
3 times, most recently
from
3014a3f
to
2893897
Compare
|
@rbarlow I think I got everything. I saw that you LGTM'd but if you don't mind, can you re-review? I will merge this to 2.6-testing as well when I merge. |
| @return: Response | ||
| """ | ||
| path = '/v2/services/status/' | ||
| return self.server.GET(path) |
There was a problem hiding this comment.
Since the bindings could be considered part of our API, we shouldn't remove this until 3.0.0. However, we could insert a DeprecationWarning.
There was a problem hiding this comment.
The path this uses is a 404. I think it's better to just remove it; I suspect it has not worked in a long while.
There was a problem hiding this comment.
@beav you are correct this should just be removed. The webcode handler is there, but it hasn't been wired up in application.py in a long time. There is no way for a web request to reach this code.
|
Re-LGTM, except that I think we should deprecate that binding rather than remove it. |
This patch returns additional data from the /status API call. Specifically, it returns a worker list, the message broker connection status, the database connection status, and the Pulp version (RHBZ #704184). There is a small behavior change to include the resource manager and scheduler as part of the worker heartbeat list. Previously, these were not included in the list. These entries are filtered out when Pulp requests a free worker. A few PEP8 fixes are included as well for files that were touched in this patch. Additionally, the `ping()` method in ServerInfoAPI has been removed; this called a URL that no longer exists.
|
Closing PR, will re-open a new one against |
|
merged in #1478 |
This patch returns additional data from the /status API call. Specifically, it
returns a worker list, the message broker connection status, the database
connection status, and the Pulp version (RHBZ #704184).
There is a small behavior change to include the resource manager and scheduler
as part of the worker heartbeat list. Previously, these were not included in
the list.