Repository navigation
fix(content): exit on database failures - #8216
Conversation
Funi1234
changed the title
| ) | ||
| except IntegrityError: | ||
| log.error(f"A content app with name {name} already exists in the database.") | ||
| exit(Arbiter.WORKER_BOOT_ERROR) |
There was a problem hiding this comment.
Should this fatal registration path also use os._exit(Arbiter.WORKER_BOOT_ERROR)? IntegrityError is handled here before the new DatabaseError handler, so it retains the exception-based exit. This path is outside the heartbeat loop's finally, so I wouldn't assume it has the same cleanup failure, but it would be useful to either use the same termination mechanism for both registration failures or cover this branch with a real-worker termination test.
| monkeypatch.setattr(AppStatus, "objects", AppStatusManager()) | ||
| mock_reset_db = Mock() | ||
| monkeypatch.setattr(Handler, "_reset_db_connection", mock_reset_db) | ||
| mock_exit = Mock(side_effect=SystemExit(Arbiter.WORKER_BOOT_ERROR)) |
There was a problem hiding this comment.
Sharing this ai-review note:
Could we add a subprocess regression test that starts the heartbeat as a detached task and leaves
os._exitunmocked? RaisingSystemExithere and directly awaiting_heartbeat()verifies the call, but doesn't exercise the process-termination behavior this fix depends on. In particular, the heartbeat'sfinallyperforms another DB operation, whose failure can replace a pendingSystemExit; real_exitskips that cleanup. The test should inject the heartbeat DB failures and failing cleanup, then assert that the child exits with code 3 within a bounded timeout. The reported Gunicorn outage/recovery scenario would also be valuable integration coverage for master termination and successful re-registration.
There was a problem hiding this comment.
Discussed in another thread, this is not the best place for a test like that to live. Resolving this thread.
| assert mock_acreate.await_count == 1 | ||
|
|
||
|
|
||
| def test_detached_heartbeat_exits_process_on_database_failure(): |
There was a problem hiding this comment.
Hmm. This looks pretty strange to me. What does it test? Would gunicorn ever decide to restructure it's process organization, this test would not catch it. Otoh it's really hard to wrap ones head around what actually happens here. Does it actually reflect the way the heartbeat code is excercised in the app?
There was a problem hiding this comment.
The content app starts the heartbeat as a detached asyncio task. The test starts that same context in a child process, makes both heartbeat writes fail, and checks that the process exits with code 3. Cleanup is also made to fail, to make sure it cannot hide the exit.
It was added because the old test mocked os._exit and called _heartbeat() directly. That only proved we called os._exit, not that the worker actually died.
It does not test Gunicorn restarting the worker or registering it again. That would need a separate integration test.
There was a problem hiding this comment.
The test as a string was going to cause confusion, I've reformatted to to be a normal function so that it is more easily readable.
There was a problem hiding this comment.
That is actually my concern. This test does not test that gunicorn terminates (and btw not restarts itself). So the only gap this test bridges is between "calling _exit" and some process actually terminating. If we stop trusting that level of python facilities, we have surpassed more than one level of insanity.
Put another way the thing this tests excersises beyond "_exit being called" is that the fake setup in the very test, that by no means of the test suite is guaranteed to reflect the actual app code in the future, also terminates. So the only answer to what needs to happen for this test to fail without the other one failing is: "Python is broken".
And therefore I would rather just scrap this test. (Yes, I know it has been suggested to add such a thing, but I believe this repository is not the right layer of the onion.)
Terminate content workers when database registration or heartbeat writes fail, so a detached heartbeat task cannot leave the worker serving without an AppStatus heartbeat.
cfde751 to
973ffc9
Compare
Backport to 3.122: 💚 backport PR created✅ Backport PR branch: Backported as #8227 🤖 @patchback |
Backport to 3.118: 💚 backport PR created✅ Backport PR branch: Backported as #8228 🤖 @patchback |
Backport to 3.105: 💚 backport PR created✅ Backport PR branch: Backported as #8229 🤖 @patchback |
4 participants
Problem
Content workers can lose database connectivity during PostgreSQL outage. content endpoint can continue returning HTTP 200 while Pulp removes worker's
AppStatusheartbeat entry. operator seesonline_content_apps: []Hub readiness never completes.Root Cause
heartbeat runs in detached asyncio task. previous failure path called:
In deployed aiohttp/Gunicorn worker, did not terminate worker process. HTTP server continued serving
/pulp/content/, so Pod remained Ready Kubernetes did not restart it.Changes
os._exit(Arbiter.WORKER_BOOT_ERROR)when initial content-app registration fails, including duplicate registration and other database failures.os._exit(Arbiter.WORKER_BOOT_ERROR)when a heartbeat write fails after the existing connection reset and second write attempt.CONTENT_APP_MAX_DB_RETRIESsetting.os._exit, injects cleanup failure, and verifies the worker exits with the Gunicorn boot-error code.worker now fails closed. Kubernetes restarts container. If PostgreSQL still unavailable, replacement process exits again; once PostgreSQL recovers, next replacement registers heartbeat normally. No registration retry loop required.
Validation
change tested against original unpatched Galaxy image using pre-PR #302 operator:
HaltServer('Worker failed boot.', 3).online_content_appsentries returned.Focused heartbeat tests cover both registration error types, heartbeat write failures, and process-boundary termination. Ruff, Black, Python compilation, and diff checks also pass.
PR #302 remains possible operator-side mitigation already-running images contain Pulpcore defect, but change fixes failure content-worker level.