[I] pgbouncer prestop command is not graceful enough [airflow]

["andrew-stein-sp (via GitHub)" <gi...@apache.org>]

andrew-stein-sp opened a new issue, #69571:
URL: https://github.com/apache/airflow/issues/69571

   ### Under which category would you file this issue?
   
   Helm chart
   
   ### Apache Airflow version
   
   2.11.0(airflow version is not applicable to this issue)
   
   ### What happened and how to reproduce it?
   
   To reproduce:
   - perform a rolling restart of the pgbouncer deployment(with multiple replicas) in an airflow cluster and kubernetes cluster under heavy load
   - observe scheduler, dag-processor, and worker pods
      - some pods will see restarts with a SQL connection error like "No route to host" or "Connection refused"
    
   
   
   ### What you think should happen instead?
   
   The prestop command used for pgbouncer in the helm chart(shown below) is problematic:
   ```  
   containerLifecycleHooks:
       preStop:
         exec:
           # Allow existing queries clients to complete within 120 seconds
           command: ["/bin/sh", "-c", "killall -INT pgbouncer && sleep 120"]
   ```
   Although `killall -INT pgbouncer` does in fact trigger a safe shutdown as expected, it exits as soon as in-flight queries drain. Once this occurs, the liveness probe terminates the pod regardless of whether the `sleep 120` has finished or not. In the event of an idle pgbouncer pod or one with no long-running queries, this happens in ~1 second or less. The container therefore stops accepting connections before the EndpointSlice/kube-proxy updates triggered by the pod entering Terminating have propagated, so new connections routed to the still-registered (but dead) pod fail with connection errors.
   
   I've fixed this by putting a short sleep command in front of `killall -INT pgbouncer` like so:
   ```  
   containerLifecycleHooks:
       preStop:
         exec:
           # Allow existing queries clients to complete within 120 seconds
           command: ["/bin/sh", "-c", "sleep 10 && killall -INT pgbouncer && sleep 20"]
   ```
   This allows the pod to transition to a `terminating` state while the pgbouncer container continues to run, which grants kubernetes more time to remove the target from the service endpoint and update the loadbalancer and kube-proxy rules. Now, if a query gets sent to the terminating pod before its been removed from the service endpoint fully, it is less likely to experience a connection failure because pgbouncer is still running and will still accept the query. 
   
   ```
   ➜  ~ kubectl get endpointslice -o json -n airflow airflow-<redacted>-pgbouncer-n57hm | grep '"terminating": true' -B2
   --
                   "ready": true,
                   "serving": true,
                   "terminating": true
   ```
   
   Also, I made the post sleep command `20` becuase there is no `TerminationGracePeriod` defined for the pgbouncer deployment in the chart, so it ends up defaulting to 30s. Basically, the `sleep 120` doesn't really do what it says and is therefore misleading to users of the chart. 
   
   ### Operating System
   
   Amazon Linux 2023(fedora)
   
   ### Deployment
   
   Official Apache Airflow Helm Chart
   
   ### Apache Airflow Provider(s)
   
   _No response_
   
   ### Versions of Apache Airflow Providers
   
   _No response_
   
   ### Official Helm Chart version
   
   1.22.0 (latest released)
   
   ### Kubernetes Version
   
   1.35
   
   ### Helm Chart configuration
   
   _No response_
   
   ### Docker Image customizations
   
   _No response_
   
   ### Anything else?
   
   _No response_
   
   ### Are you willing to submit PR?
   
   - [x] Yes I am willing to submit a PR!
   
   ### Code of Conduct
   
   - [x] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] pgbouncer prestop command is not graceful enough [airflow]

["Miretpl (via GitHub)" <gi...@apache.org>]

Miretpl commented on issue #69571:
URL: https://github.com/apache/airflow/issues/69571#issuecomment-4929644837

   Thanks for creating this issue!
   
   > Also, I made the post sleep command 20 becuase there is no TerminationGracePeriod defined for the pgbouncer deployment in the chart, so it ends up defaulting to 30s. Basically, the sleep 120 doesn't really do what it says and is therefore misleading to users of the chart.
   
   IMO, sleep after is pretty much useless; if it is shorter (total command exec time) than the grace period, it doesn't have any effect; if it is longer, it will make the grace period longer by 2 sec, so basically it could be deleted (unless I'm missing something).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] pgbouncer prestop command is not graceful enough [airflow]

["PreethamSanji (via GitHub)" <gi...@apache.org>]

PreethamSanji commented on issue #69571:
URL: https://github.com/apache/airflow/issues/69571#issuecomment-4932018436

   I'd like to work on this issue


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org