Implementing graceful shutdown #105

zainkabani · 2022-08-02T21:52:26Z

This PR attempts to implement a graceful shutdown mechanism for pgcat like pgbouncer.

Initiated on SIGINT signal.
Waits for clients to finish work then exits
Respects transactions and allows them to complete

levkk · 2022-08-02T21:58:18Z

src/main.rs

-        _ = signal::ctrl_c() => (),
-        _ = term_signal.recv() => (),
-    };
+    // initiate graceful shutdown sequence on sig int


Suggested change

// initiate graceful shutdown sequence on sig int

// Initiate graceful shutdown sequence on SIGINT

zainkabani · 2022-08-02T21:59:09Z

Current issue is that a client can be connected to pgcat and not be sending any messages and pgcat will be waiting for this client to complete it's work. Need some ideas to help solve this

levkk · 2022-08-02T21:59:24Z

src/main.rs

-    let mut term_signal = unix_signal(SignalKind::terminate()).unwrap();
-
-    tokio::select! {
-        _ = signal::ctrl_c() => (),


Curious why you removed the ctrl-c and the SIGTERM handler? SIGTERM is commonly used in Docker containers to stop a container.

Pgbouncer interprets sigterm and sigint differently. since the sigint is graceful shutdown we want to catch and handle that. for sigterm, which is immediate shutdown we don't need to do anything special

SIGINT Safe shutdown. Same as issuing PAUSE and SHUTDOWN on the console. SIGTERM Immediate shutdown. Same as issuing SHUTDOWN on the console.

I'm curious about the reasoning here. In Kuberentes SIGTERM is meant to initiate a graceful shutdown. https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/

levkk · 2022-08-02T22:01:05Z

src/main.rs

+    stream.recv().await;
+    client::SHUTTING_DOWN.store(true, Ordering::Relaxed);
+    warn!("Got SIGINT, waiting for client connection drain now");
+    wg.wait();


I'm wondering if just save tokio::task::spawn handles in a Vec in main.rs and then waiting for them all to exit task.await is best?

levkk · 2022-08-02T22:03:55Z

Cargo.toml

@@ -31,3 +31,4 @@ base64 = "0.13"
 stringprep = "0.1"
 tokio-rustls = "0.23"
 rustls-pemfile = "1"
+wg = "0.1.0"


Based on this, it doesn't look like a well-known crate. Are we sure we need it? I think tokio should give us the tools we need to do this task.

levkk · 2022-08-02T22:08:15Z

Current issue is that a client can be connected to pgcat and not be sending any messages and pgcat will be waiting for this client to complete it's work. Need some ideas to help solve this

Interesting problem. What about using select! while listening for client messages, and if we receive a shutdown signal, we close the connection and exit the loop?

Tokio docs have some ideas for us: https://tokio.rs/tokio/topics/shutdown

zainkabani · 2022-08-05T00:59:14Z

Used channel transmitter shutdown logic found here: https://tokio.rs/tokio/topics/shutdown

drdrsh · 2022-08-05T16:47:26Z

Cargo.toml

@@ -30,4 +30,4 @@ sha2 = "0.10"
 base64 = "0.13"
 stringprep = "0.1"
 tokio-rustls = "0.23"
-rustls-pemfile = "1"
+rustls-pemfile = "1"


Can you please add a terminal new line

src/client.rs

levkk · 2022-08-08T15:24:47Z

src/main.rs

    // Client connection loop.
    tokio::task::spawn(async move {
+        // Creates event subscriber for shutdown event, this is dropped when shutdown event is broadcast
+        let mut listener_rx = shutdown_event_tx_clone.subscribe();


Please rename to shutdown_event_rx because it can be confused with TCP listener by the reader.

levkk · 2022-08-08T15:26:54Z

src/main.rs

+    // Closes transmitter
+    drop(shutdown_event_tx);
+
+    loop {


Why are you looping?

There should be two events that the receiver gets, the 1st is the event sent by shutdown_event_tx.send(()).unwrap(); and then the second event would be when all senders are closed and there is an error on the receive channel. made it a loop so that the timeout could be applied to all the operations.

levkk · 2022-08-08T15:28:04Z

tests/python/tests.py

+    cleanup_conn(conn, cur)
+
+
+def test_shutdown_logic():


We use bash for these kind of tests, why Python?

Since we're operating on the same postgres connection and doing things in between queries and transactions it felt easiest to do it in python.

levkk

LGTM

drdrsh · 2022-08-08T23:02:43Z

src/messages.rs

-    let mut bytes = BytesMut::with_capacity(5);
+    let mut bytes = BytesMut::with_capacity(
+        mem::size_of::<u8>() + mem::size_of::<i32>() + mem::size_of::<u8>(),
+    );

    bytes.put_u8(b'Z');
    bytes.put_i32(5);


@zainkabani I think you may want to update this 5 to also be mem::size_of::<u8>() + mem::size_of::<i32>() + mem::size_of::<u8>()

zainkabani added 2 commits August 2, 2022 17:51

Initial commit for graceful shutdown

dba369a

fmt

72c6622

levkk reviewed Aug 2, 2022

View reviewed changes

zainkabani added 4 commits August 4, 2022 20:32

Add .vscode to gitignore

aa593ed

Updates shutdown logic to use channels

029429d

fmt

67a81f3

fmt

d667b17

drdrsh reviewed Aug 5, 2022

View reviewed changes

zainkabani and others added 7 commits August 5, 2022 19:02

Adds shutdown timeout

324b539

Fmt and updates tomls

6ed4eb1

Updates readme

f667e7a

fmt and updates log levels

78a1d70

Update python tests to test shutdown

3737f06

Merge branch 'main' into graceful-shutdown

b01b54c

merge changes

cec0f1f

levkk reviewed Aug 8, 2022

View reviewed changes

src/client.rs Show resolved Hide resolved

levkk reviewed Aug 8, 2022

View reviewed changes

zainkabani added 5 commits August 8, 2022 12:57

Rename listener rx and update bash to be in line with master

863ba8c

Update python test bash script ordering

335ad62

Adds error response message before shutdown

df08b7f

Add details on shutdown event loop

35d3346

Fixes response length for error

ab4e669

zainkabani added 3 commits August 8, 2022 17:43

Adds handler for sigterm

72d7492

Uses ready for query function and fixes number of bytes

e1620dd

fmt

b018f41

levkk approved these changes Aug 8, 2022

View reviewed changes

levkk merged commit 3719c22 into postgresml:main Aug 8, 2022

drdrsh reviewed Aug 8, 2022

View reviewed changes

zainkabani deleted the graceful-shutdown branch August 9, 2022 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing graceful shutdown #105

Implementing graceful shutdown #105

zainkabani commented Aug 2, 2022 •

edited

Loading

levkk Aug 2, 2022

zainkabani commented Aug 2, 2022

levkk Aug 2, 2022

zainkabani Aug 5, 2022

adriangb Jun 21, 2024

levkk Aug 2, 2022 •

edited

Loading

levkk Aug 2, 2022 •

edited

Loading

levkk commented Aug 2, 2022 •

edited

Loading

zainkabani commented Aug 5, 2022

drdrsh Aug 5, 2022

levkk Aug 8, 2022

levkk Aug 8, 2022

zainkabani Aug 8, 2022

levkk Aug 8, 2022

zainkabani Aug 8, 2022

levkk left a comment

drdrsh Aug 8, 2022

	// initiate graceful shutdown sequence on sig int
	// Initiate graceful shutdown sequence on SIGINT

Implementing graceful shutdown #105

Implementing graceful shutdown #105

Conversation

zainkabani commented Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

zainkabani commented Aug 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkk Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

levkk Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

levkk commented Aug 2, 2022 • edited Loading

zainkabani commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levkk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zainkabani commented Aug 2, 2022 •

edited

Loading

levkk Aug 2, 2022 •

edited

Loading

levkk Aug 2, 2022 •

edited

Loading

levkk commented Aug 2, 2022 •

edited

Loading