Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove rack-timeout and integrate slowpoke #1454

Merged
merged 2 commits into from Mar 13, 2023
Merged

Conversation

Pralish
Copy link
Collaborator

@Pralish Pralish commented Mar 13, 2023

Addresses:
#1434
#1419 (comment)

@donrestarone
Copy link
Contributor

blocks: #1419 please merge first

@@ -129,4 +129,5 @@
# config.active_record.database_selector = { delay: 2.seconds }
# config.active_record.database_resolver = ActiveRecord::Middleware::DatabaseSelector::Resolver
# config.active_record.database_resolver_context = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session
config.slowpoke.timeout = ENV['VIOLET_SERVICE_TIMEOUT'].to_i.nonzero? || 15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this to an initializer so it works in all environments

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1454.violet-test.net

@donrestarone
Copy link
Contributor

@Pralish I did a spot of research on slowpoke, we should handle the following cases:

  1. send exception notification when timeouts happen (we do this currently with rack-timeout): https://github.com/ankane/slowpoke#production
  2. customizing timeout behaviour: https://github.com/ankane/slowpoke#safer-service-timeouts

@donrestarone
Copy link
Contributor

donrestarone commented Mar 13, 2023

🧪 Test plan

dokku apps:list

list apps

ubuntu@ip-172-31-5-222:~$ dokku apps:list
=====> My Apps
review-1382
review-1433
review-1444
review-1446
review-1454
review-1455
violet-rails

set a really short timeout

set timeout for this PR/APP (#1454)

ubuntu@ip-172-31-5-222:~$ dokku config:set review-1454 VIOLET_SERVICE_TIMEOUT=1
-----> Setting config vars
       VIOLET_SERVICE_TIMEOUT:  1
-----> Restarting app review-1454
-----> Releasing review-1454...
-----> Checking for predeploy task
       No predeploy task found, skipping
-----> Checking for release task
-----> Executing release task from Procfile: bundle exec rails db:migrate
=====> Start of review-1454 release task (140ee7ce5) output
 !     No stripe.com API key was configured for environment production! this application will be
 !     unable to interact with stripe.com. You can set your API key with either the environment
 !     variable `STRIPE_SECRET_KEY` (recommended) or by setting `config.stripe.secret_key` in your
       Migrating prashant tenant
       Migrating root tenant
 !     environment file directly.
=====> End of review-1454 release task (140ee7ce5) output
-----> App Procfile file found
=====> Processing deployment checks
       No CHECKS file found. Simple container checks will be performed.
       For more efficient zero downtime deployments, create a CHECKS file. See https://dokku.com/docs/deployment/zero-downtime-deploys/ for examples
-----> Deploying review-1454 via the docker-local scheduler...
-----> Deploying web (count=1)
       Attempting pre-flight checks (web.1)
       Waiting for 10 seconds (web.1)
       Default container check successful (web.1)
       Scheduling old container shutdown in 60 seconds (web.1)
-----> Deploying release (count=0)
-----> Deploying worker (count=1)
       Attempting pre-flight checks (worker.1)
       Waiting for 10 seconds (worker.1)
       Default container check successful (worker.1)
       Scheduling old container shutdown in 60 seconds (worker.1)
-----> Running post-deploy
-----> Configuring review-1454.violet-test.net...(using built-in template)
-----> Creating https nginx.conf
       Enabling HSTS
       Reloading nginx
-----> Renaming containers
       Found previous container(s) (785287facf83) named review-1454.web.1
       Renaming container (785287facf83) review-1454.web.1 to review-1454.web.1.1678713046
       Renaming container review-1454.web.1.upcoming-30795 (9fb34894c3c4) to review-1454.web.1
       Found previous container(s) (a0c1cfd3a17f) named review-1454.worker.1
       Renaming container (a0c1cfd3a17f) review-1454.worker.1 to review-1454.worker.1.1678713046
       Renaming container review-1454.worker.1.upcoming-530 (11b4b48ebf56) to review-1454.worker.1
-----> Checking for postdeploy task
       No postdeploy task found, skipping
-----> Updated schedule file
-----> Shutting down old containers in 60 seconds
=====> Application deployed:
       http://review-1454.violet-test.net
       https://review-1454.violet-test.net

tail the logs

dokku logs -t review-1454

visit an expensive page

https://review-1454.violet-test.net/admin

inspect log for thread timeout and restart

2023-03-13T13:15:29.765040070Z app[web.1]: [c29b479a-452e-48a6-addf-14ce123ddc90] Rack::Timeout::RequestTimeoutError (Request waited 7ms, then ran for longer than 1000ms ):
2023-03-13T13:15:29.765044386Z app[web.1]: [c29b479a-452e-48a6-addf-14ce123ddc90]   
2023-03-13T13:15:29.765048742Z app[web.1]: [c29b479a-452e-48a6-addf-14ce123ddc90] app/views/layouts/comfy/admin/cms/_body.html.haml:5
2023-03-13T13:15:29.765052648Z app[web.1]: [c29b479a-452e-48a6-addf-14ce123ddc90] app/views/layouts/comfy/admin/cms.html.haml:16
2023-03-13T13:15:29.766559263Z app[web.1]: - Gracefully stopping, waiting for requests to finish

see timeout on the browser + email notification

Screen Shot 2023-03-13 at 9 13 09 AM

see that the server was terminated, and restarted to restore service

Screen Shot 2023-03-13 at 9 18 23 AM

@Pralish
Copy link
Collaborator Author

Pralish commented Mar 13, 2023

@donrestarone

send exception notification when timeouts happen (we do this currently with rack-timeout):

Isn't this being handled by our violet's exception notifier service?

@donrestarone
Copy link
Contributor

@Pralish yup, forgot to mention I got spammed by exception notification emails when I tested this

@github-actions
Copy link

Deployed review-app can be viewed at https://review-1454.violet-test.net

@donrestarone donrestarone changed the base branch from master to rc March 13, 2023 16:48
@donrestarone donrestarone merged commit df61ced into rc Mar 13, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants