Handle SIGTERM received by server gracefully #7948

ddelange · 2022-12-20T06:34:32Z

Closes #7938 by adding a SIGTERM signal handler

Example

Checklist

This pull request references any related issue by including "closes <link to issue>"
- If no issue exists and your change is not a small fix, please create an issue first.
This pull request includes tests or only affects documentation.
This pull request includes a label categorizing the change e.g. fix, feature, enhancement

netlify · 2022-12-20T06:35:05Z

✅ Deploy Preview for prefect-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`8763521`
🔍 Latest deploy log	https://app.netlify.com/sites/prefect-docs/deploys/63ff91d5383e0b0007f932a3
😎 Deploy Preview	https://deploy-preview-7948--prefect-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

ddelange · 2022-12-20T06:41:47Z

@madkinsz how could I add a test Call SIGTERM handler expect a SIGTERM? I guess a new test block is needed, but what to change? It's not obvious to me how to choose which signal is sent, there's just the assertion 🤔

https://github.com/PrefectHQ/prefect/blob/2.7.3/tests/utilities/test_processutils.py#L136-L139

serinamarie · 2022-12-23T18:12:04Z

Hi @ddelange, with the change that you made, did you see a difference in shutdown behavior? I tried this locally and when I SIGTERM the Uvicorn process I don't see a change in effect.

ddelange · 2022-12-23T18:39:37Z

No I have not tested it actually, just a guess and question from the mobile website 😅

serinamarie · 2022-12-23T20:25:22Z

Hi @ddelange, no worries, as it turns out, sending a SIGTERM to the prefect process with your change worked as expected! However, we probably don't want forwarding of SIGTERM inside kill_on_interrupt, but instead to forward a SIGTERM directly. This in turn would mean creating a new test file inside of tests/cli/ called test_orion_start.py 🙂 instead of in test_processutils.py.

ddelange · 2022-12-23T20:58:33Z

Any help with such a test would be highly appreciated, from the code comments in the current test: I'm not sure how to properly test the 'end to end' variation with pytest 🤔

zanieb · 2022-12-23T21:19:51Z

src/prefect/utilities/processutils.py

@@ -337,3 +337,4 @@ def kill_process(*args):
            os.kill(pid, signal.SIGKILL)

    signal.signal(signal.SIGINT, stop_process)
+    signal.signal(signal.SIGTERM, stop_process)


We'll want a new utility called forward_signal(signum: int, pid: int) that'd look roughly like

previous_signal_handler = ... def forward_signal os.kill(pid, signum) previous_signal_handler(..,) signal.signal(sugnum, forward_signal)

You'll want to move call this from the prefect orion start command

To test the change to the CLI, you can run prefect orion start in a subprocess (using popen or similar) then send a SIGTERM to it. It should then exit gracefully.

testing sounds good.

as for your utility suggestion, i think im stumbling over something. probably insufficient understanding of signals :)

do you mean a generalisation of the utility? sigterm calls signint calls sigkill? so recursion?

i think i dont understand previous_signal_handler 🤔

is this something you'd like to implement, and could tag me on so i can learn?

and by testing "exit gracefully", do you mean to poll the subprocess exit code and assert it is 1 within a second or two?

do i understand correctly from the comment in the code that you tried builtin anyio signal handling, but it didn't work nicely together with click?

Hi 👋 I took the time to put together the code and tests as I think you intended. The tests do a wait for prefect orion start subprocess to spin up a uvicorn worker, send two signals shortly after each other, and assert by the process output that the correct signals were received:

[ b'\n', b' ___ ___ ___ ___ ___ ___ _____ ___ ___ ___ ___ _ _\n', b'| _ \\ _ \\ __| __| __/ __|_ _| / _ \\| _ \\_ _/ _ \\| \\| |\n', b'| _/ / _|| _|| _| (__ | | | (_) | /| | (_) | .` |\n', b'|_| |_|_\\___|_| |___\\___| |_| \\___/|_|_\\___\\___/|_|\\_|\n', b'\n', b'Configure Prefect to communicate with the server with:\n', b'\n', b' prefect config set PREFECT_API_URL=http://0.0.0.0:4200/api\n', b'\n', b'View the API reference documentation at http://0.0.0.0:4200/docs\n', b'\n', b"The dashboard is not built. It looks like you're on a development version.\n", b'See `prefect dev` for development commands.\n', b'\n', b'\n', b'\n', b'11:15:44.690 | INFO | uvicorn.error - Started server process [89342]\n', b'11:15:44.693 | INFO | uvicorn.error - Waiting for application startup.\n', b'11:15:44.993 | INFO | prefect.orion - Scheduler service scheduled to start in-app\n', b'11:15:44.993 | INFO | prefect.orion - RecentDeploymentsScheduler service scheduled to start in-app\n', b'11:15:44.994 | INFO | prefect.orion - MarkLateRuns service scheduled to start in-app\n', b'11:15:44.994 | INFO | prefect.orion - FailExpiredPauses service scheduled to start in-app\n', b'11:15:44.995 | INFO | prefect.orion - Telemetry service scheduled to start in-app\n', b'11:15:44.995 | INFO | prefect.orion - FlowRunNotifications service scheduled to start in-app\n', b'11:15:45.013 | INFO | uvicorn.error - Application startup complete.\n', b'11:15:45.017 | INFO | uvicorn.error - Uvicorn running on http://0.0.0.0:4200 (Press CTRL+C to quit)\n', b'\n', b'Sending SIGTERM to Orion (PID 89342)...\n', b'11:15:45.107 | INFO | prefect.orion.services.recentdeploymentsscheduler - Scheduled 0 runs.\n', b'\n', b'Sending SIGKILL to Orion (PID 89342)...\n', b'Orion stopped!\n' ]

added another test for the graceful shutdown. it looks really graceful 😄

___ ___ ___ ___ ___ ___ _____ ___ ___ ___ ___ _ _ | _ \ _ \ __| __| __/ __|_ _| / _ \| _ \_ _/ _ \| \| | | _/ / _|| _|| _| (__ | | | (_) | /| | (_) | .` | |_| |_|_\___|_| |___\___| |_| \___/|_|_\___\___/|_|\_| Configure Prefect to communicate with the server with: prefect config set PREFECT_API_URL=http://0.0.0.0:4200/api View the API reference documentation at http://0.0.0.0:4200/docs The dashboard is not built. It looks like you're on a development version. See `prefect dev` for development commands. 11:38:26.690 | INFO | uvicorn.error - Started server process [90293] 11:38:26.693 | INFO | uvicorn.error - Waiting for application startup. 11:38:27.006 | INFO | prefect.orion - Scheduler service scheduled to start in-app 11:38:27.007 | INFO | prefect.orion - RecentDeploymentsScheduler service scheduled to start in-app 11:38:27.007 | INFO | prefect.orion - MarkLateRuns service scheduled to start in-app 11:38:27.008 | INFO | prefect.orion - FailExpiredPauses service scheduled to start in-app 11:38:27.008 | INFO | prefect.orion - Telemetry service scheduled to start in-app 11:38:27.009 | INFO | prefect.orion - FlowRunNotifications service scheduled to start in-app 11:38:27.029 | INFO | uvicorn.error - Application startup complete. 11:38:27.031 | INFO | uvicorn.error - Uvicorn running on http://0.0.0.0:4200 (Press CTRL+C to quit) Sending SIGTERM to Orion (PID 90293)... 11:38:27.147 | INFO | uvicorn.error - Shutting down 11:38:27.152 | INFO | prefect.orion.services.recentdeploymentsscheduler - Scheduled 0 runs. 11:38:27.155 | INFO | prefect.orion.services.scheduler - Scheduled 0 runs. 11:38:27.156 | INFO | prefect.orion.services.failexpiredpauses - Finished monitoring for late runs. 11:38:27.158 | INFO | prefect.orion.services.marklateruns - Finished monitoring for late runs. 11:38:27.161 | INFO | prefect.orion - FlowRunNotifications service stopped! 11:38:27.250 | INFO | uvicorn.error - Waiting for application shutdown. 11:38:27.781 | INFO | prefect.orion - Telemetry service stopped! 11:38:28.159 | INFO | prefect.orion - RecentDeploymentsScheduler service stopped! 11:38:28.160 | INFO | prefect.orion - Scheduler service stopped! 11:38:28.161 | INFO | prefect.orion - FailExpiredPauses service stopped! 11:38:28.162 | INFO | prefect.orion - MarkLateRuns service stopped! 11:38:28.175 | INFO | uvicorn.error - Application shutdown complete. 11:38:28.176 | INFO | uvicorn.error - Finished server process [90293] Orion stopped!

Signed-off-by: ddelange <[email protected]>

ddelange · 2023-01-04T11:38:18Z

@serinamarie @madkinsz ready for review 👍

zanieb

Thanks!

ddelange · 2023-01-06T21:34:56Z

did I pull a flakey test?

zanieb · 2023-01-06T22:13:46Z

Looks like tests/cli/test_start_orion.py::TestUvicornSignalForwarding::test_sigterm_sends_sigterm_directly - subprocess.TimeoutExpired: Command '['prefect', 'orion', 'start', '--host', '0.0.0.0', '--port', '4200', '--log-level', 'INFO']' timed out after 2 seconds is failing/hanging.

ddelange · 2023-01-07T08:32:05Z

thanks, somehow missed that. bumped the timeout, which is more of a failsafe anyway

ddelange · 2023-01-07T09:42:06Z

ran that particular test again on my machine, from the point of sending SIGTERM it needs a little over a second to reach Orion stopped!

zanieb · 2023-02-22T15:25:23Z

@ddelange thanks for updating this with main! I appreciate that you're still on top of it. @bunchesofdonald would you have time to review this?

ddelange · 2023-02-24T10:53:55Z

wohoo we're green 💥

* 'main' of https://github.com/prefecthq/prefect: Bump @playwright/test from 1.30.0 to 1.31.1 in /ui (PrefectHQ#8663) Bump @prefecthq/prefect-ui-library from 1.1.9 to 1.1.10 in /ui (PrefectHQ#8661) Bump vite from 4.1.2 to 4.1.4 in /ui (PrefectHQ#8662) Bump eslint from 8.34.0 to 8.35.0 in /ui (PrefectHQ#8664) Fix of a typo - removed repeated word (PrefectHQ#8654) Enable DefaultAzureCredential authentication for Azure filesystem block (PrefectHQ#7513) Improve supervisor repr for debugging (PrefectHQ#8633)

ddelange · 2023-03-01T06:29:10Z

@madkinsz can it be that these new tests are making existing tests flaky?

zanieb · 2023-03-01T17:27:01Z

@ddelange I'm not sure, I took on #8680 / #8679 / #8681 in an attempt to fix some flakes I saw here.

src/prefect/utilities/processutils.py

zanieb · 2023-03-01T18:27:18Z

Thanks for your patience!

…eful-agent * 'main' of https://github.com/prefecthq/prefect: BugFix: Add information about task inputs (PrefectHQ#8295) Handle SIGTERM received by server gracefully (PrefectHQ#7948)

Handle SIGTERM signal gracefully

5a85027

ddelange requested a review from zanieb as a code owner December 20, 2022 06:34

zanieb reviewed Dec 23, 2022

View reviewed changes

ddelange added 3 commits December 27, 2022 11:32

PR Suggestions

12e8307

Signed-off-by: ddelange <[email protected]>

Add graceful SIGTERM shutdown test

338bbd0

Signed-off-by: ddelange <[email protected]>

Add returncode assertions

3352ac7

Signed-off-by: ddelange <[email protected]>

ddelange force-pushed the patch-1 branch from a73c7a8 to 3352ac7 Compare December 27, 2022 10:59

Merge branch 'main' into patch-1

4da8b06

ddelange requested a review from zanieb January 4, 2023 10:48

Merge branch 'main' into patch-1

8908490

zanieb approved these changes Jan 6, 2023

View reviewed changes

zanieb added the enhancement An improvement of an existing feature label Jan 6, 2023

ddelange added 3 commits January 6, 2023 21:42

pre-commit run --all-files

faa9052

Merge branch 'main' into patch-1

39c27ed

Merge branch 'main' into patch-1

513678d

Bump SHUTDOWN_TIMEOUT for tests

7fa5fa8

ddelange requested review from serinamarie and peytonrunyan as code owners January 7, 2023 08:30

Merge branch 'main' into patch-1

b6859d0

zanieb changed the title ~~Handle SIGTERM received by orion-server gracefully~~ Handle SIGTERM received by server gracefully Feb 22, 2023

ddelange added 3 commits February 23, 2023 14:14

Merge branch 'main' into patch-1

447f870

Merge leftover

93bf35b

Add docstring

0759da1

ddelange mentioned this pull request Feb 24, 2023

Handle SIGTERM received by agent gracefully ddelange/prefect#17

Closed

ddelange added 2 commits February 24, 2023 10:40

Typo

cca9d29

Merge branch 'main' into patch-1

f0a8243

zanieb and others added 6 commits February 24, 2023 10:15

Merge branch 'main' into patch-1

1429b1f

Merge branch 'main' into patch-1

f115dbd

Merge branch 'main' into patch-1

b9a4d82

Allow more ports

4062d34

Merge branch 'main' into patch-1

094e661

ddelange added 3 commits March 1, 2023 09:54

Merge branch 'main' into patch-1

f6df82d

Increase verbosity

d1eb1d8

Amend

82ed35f

ddelange mentioned this pull request Mar 1, 2023

Subprocess Killed Task is Not Marked as Failed #6024

Open

Merge branch 'main' into patch-1

4be41f2

zanieb reviewed Mar 1, 2023

View reviewed changes

src/prefect/utilities/processutils.py Show resolved Hide resolved

ddelange commented Mar 1, 2023

View reviewed changes

src/prefect/utilities/processutils.py Show resolved Hide resolved

Add back comment

8763521

zanieb merged commit c3dd617 into PrefectHQ:main Mar 1, 2023

ddelange mentioned this pull request Mar 2, 2023

Handle SIGTERM received by agent gracefully #8691

Merged

3 tasks

br3ndonland mentioned this pull request Jun 4, 2023

Test workers encode/uvicorn#1995

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle SIGTERM received by server gracefully #7948

Handle SIGTERM received by server gracefully #7948

ddelange commented Dec 20, 2022 •

edited by zanieb

Loading

netlify bot commented Dec 20, 2022 •

edited

Loading

ddelange commented Dec 20, 2022

serinamarie commented Dec 23, 2022

ddelange commented Dec 23, 2022

serinamarie commented Dec 23, 2022 •

edited

Loading

ddelange commented Dec 23, 2022

zanieb Dec 23, 2022 •

edited

Loading

zanieb Dec 23, 2022

ddelange Dec 23, 2022 •

edited

Loading

ddelange Dec 23, 2022

ddelange Dec 23, 2022

ddelange Dec 24, 2022

ddelange Dec 27, 2022 •

edited

Loading

ddelange Dec 27, 2022 •

edited

Loading

ddelange commented Jan 4, 2023

zanieb left a comment

ddelange commented Jan 6, 2023

zanieb commented Jan 6, 2023

ddelange commented Jan 7, 2023

ddelange commented Jan 7, 2023

zanieb commented Feb 22, 2023

ddelange commented Feb 24, 2023

ddelange commented Mar 1, 2023

zanieb commented Mar 1, 2023

zanieb commented Mar 1, 2023

Handle SIGTERM received by server gracefully #7948

Handle SIGTERM received by server gracefully #7948

Conversation

ddelange commented Dec 20, 2022 • edited by zanieb Loading

Example

Checklist

netlify bot commented Dec 20, 2022 • edited Loading

✅ Deploy Preview for prefect-docs ready!

ddelange commented Dec 20, 2022

serinamarie commented Dec 23, 2022

ddelange commented Dec 23, 2022

serinamarie commented Dec 23, 2022 • edited Loading

ddelange commented Dec 23, 2022

zanieb Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

zanieb Dec 23, 2022

Choose a reason for hiding this comment

ddelange Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

ddelange Dec 23, 2022

Choose a reason for hiding this comment

ddelange Dec 23, 2022

Choose a reason for hiding this comment

ddelange Dec 24, 2022

Choose a reason for hiding this comment

ddelange Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

ddelange Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

ddelange commented Jan 4, 2023

zanieb left a comment

Choose a reason for hiding this comment

ddelange commented Jan 6, 2023

zanieb commented Jan 6, 2023

ddelange commented Jan 7, 2023

ddelange commented Jan 7, 2023

zanieb commented Feb 22, 2023

ddelange commented Feb 24, 2023

ddelange commented Mar 1, 2023

zanieb commented Mar 1, 2023

zanieb commented Mar 1, 2023

ddelange commented Dec 20, 2022 •

edited by zanieb

Loading

netlify bot commented Dec 20, 2022 •

edited

Loading

serinamarie commented Dec 23, 2022 •

edited

Loading

zanieb Dec 23, 2022 •

edited

Loading

ddelange Dec 23, 2022 •

edited

Loading

ddelange Dec 27, 2022 •

edited

Loading

ddelange Dec 27, 2022 •

edited

Loading